Create file open hints on IOContext to replace ReadAdvice#14482
Merged
ChrisHegarty merged 14 commits intoapache:mainfrom Apr 30, 2025
Merged
Create file open hints on IOContext to replace ReadAdvice#14482ChrisHegarty merged 14 commits intoapache:mainfrom
ChrisHegarty merged 14 commits intoapache:mainfrom
Conversation
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
thecoop
commented
Apr 14, 2025
thecoop
force-pushed
the
iocontext-split
branch
3 times, most recently
from
7a36602 to
440ddf1
Compare
Contributor
ChrisHegarty
left a comment
ChrisHegarty
left a comment
There was a problem hiding this comment.
I like this change. I can see how the usage sites withHints(... are very convenient and readable. This comes at the cost of validateIOContext, which I think is ok.
lucene/core/src/test/org/apache/lucene/store/TestDirectory.java
Outdated
Show resolved
Hide resolved
thecoop
force-pushed
the
iocontext-split
branch
from
440ddf1 to
808e7fa
Compare
Contributor
Author
|
To stop this PR getting too big, I suggest merging this here (which doesn't change any existing behaviour), and work on updating uses of IOContext to use hints rather than ReadAdvice in another PR |
This was referenced Apr 16, 2025
Contributor
Author
rmuir
reviewed
Apr 22, 2025
rmuir
reviewed
Apr 22, 2025
jpountz
reviewed
Apr 22, 2025
Contributor
There was a problem hiding this comment.
I like the general idea of tracking more information about what is being read on IOContext and transferring the ownership of computing the appropriate read advice from codecs to directories. I left some comments on some implementation details that confused me a bit (in addition to the validation logic like Robert).
jpountz
approved these changes
Apr 29, 2025
Contributor
jpountz
left a comment
jpountz
left a comment
There was a problem hiding this comment.
I left some small comments but the change otherwise looks good to me!
lucene/core/src/java/org/apache/lucene/store/DefaultIOContext.java
Outdated
Show resolved
Hide resolved
thecoop
force-pushed
the
iocontext-split
branch
from
92daae4 to
86e7935
Compare
github-project-automation
bot
moved this from Open
to Merged
in OpenSearch Lucene & Core Performance Tracking
ChrisHegarty
pushed a commit
that referenced
this pull request
Apr 30, 2025
Refactor IOContext and create FileOpenHint to specify how files are likely to be accessed once opened. This is the first PR of several to move from specifying ReadAdvice directly, to specifying the file usage context as hints and the ReadAdvice being inferred from the hints inside Directory implementations. Relates #14422
weizijun
added a commit
to weizijun/lucene
that referenced
this pull request
Apr 30, 2025
* main: (51 commits) Fix ECJ compiler config for Java 24 (also affects Eclipse IDE) Correct shebang in gradlew. apache#14592 Overwrite gradlew scripts with up-to-date ones and apply Lucene customizations. (apache#14592) Remove abstractions of MMapDirectory and its "fake" MR-JAR support and move it to main sourceSet (vectors untouched) (apache#14564) deps(java): bump org.gradle.toolchains.foojay-resolver-convention (apache#14573) deps(java): bump com.gradle.develocity from 3.18.2 to 4.0.1 (apache#14585) deps(java): bump nl.littlerobots.version-catalog-update (apache#14568) Bump expected base Java version to 24. apache#14533 Create file open hints on IOContext to replace ReadAdvice (apache#14482) deps(java): bump com.github.ben-manes.versions from 0.51.0 to 0.52.0 (apache#14574) deps(java): bump org.owasp.dependencycheck from 7.2.0 to 12.1.1 (apache#14584) deps(java): bump org.eclipse.jgit:org.eclipse.jgit (apache#14579) deps(java): bump com.diffplug.spotless from 6.9.1 to 7.0.3 (apache#14588) deps(java): bump com.gradle.common-custom-user-data-gradle-plugin (apache#14587) deps(java): bump net.ltgt.errorprone from 4.1.0 to 4.2.0 (apache#14567) deps(java): bump org.locationtech.jts:jts-core from 1.17.0 to 1.20.0 (apache#14586) deps(java): bump org.apache.opennlp:opennlp-tools from 2.5.3 to 2.5.4 (apache#14580) deps(java): bump asm from 9.6 to 9.8 (apache#14578) deps(java): bump commons-codec:commons-codec from 1.17.2 to 1.18.0 (apache#14581) deps(java): bump net.java.dev.javacc:javacc from 7.0.12 to 7.0.13 (apache#14576) ... # Conflicts: # lucene/CHANGES.txt
ChrisHegarty
pushed a commit
that referenced
this pull request
May 9, 2025
Followon from #14482. ReadAdvice is now only really used by MMapDirectory, no longer part of Directory. This PR doesn't change any behaviour, it just replicates the ReadAdvice that would be used using hints + MMapDirectory.toReadAdvice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor
IOContextand createFileOpenHintto specify how files are likely to be accessed once opened. This is the first PR of several to move from specifyingReadAdvicedirectly, to specifying the file usage context as hints and theReadAdvicebeing inferred from the hints insideDirectoryimplementations.Relates #14422