Skip to content

Reapply "Update the IOContext rather than the ReadAdvice on IndexInput (#14702)"#14844

Merged
thecoop merged 5 commits intoapache:mainfrom
thecoop:reapply-readadvice-iocontext
Aug 8, 2025
Merged

Reapply "Update the IOContext rather than the ReadAdvice on IndexInput (#14702)"#14844
thecoop merged 5 commits intoapache:mainfrom
thecoop:reapply-readadvice-iocontext

Conversation

@thecoop
Copy link
Contributor

@thecoop thecoop commented Jun 25, 2025

Reapply #14702

@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2025

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

@thecoop
Copy link
Contributor Author

thecoop commented Jun 25, 2025

@dweiss The tests here all pass, what's the original failure with #14702 ?

@dweiss
Copy link
Contributor

dweiss commented Jun 25, 2025

Don't know if you're subscribed -
https://lists.apache.org/list.html?builds@lucene.apache.org

Tests are randomized - sometimes a different reshuffle of components may not succeed after a patch. Here's an example:
https://ci-builds.apache.org/job/Lucene/job/Lucene-Check-main/13302/console

I haven't tried if it reproduces or not - I'm very limited today - seems like the builds have gone back to normal after I reverted the patch though, so something is not right?

@thecoop
Copy link
Contributor Author

thecoop commented Jun 25, 2025 •
edited
Loading

Yes, that's caused by this PR... It reproduces, but only intermittently...

@dweiss
Copy link
Contributor

dweiss commented Jun 25, 2025

I get a high failure ratio with this:

./gradlew :lucene:core:test  --tests "org.apache.lucene.index.TestConcurrentMergeScheduler.testNoWaitClose" -Ptests.file.encoding=US-ASCII -Ptests.forceintegervectors=true -Ptests.gui=true -Ptests.haltonfailure=false -Ptests.jvmargs= -Ptests.jvms=4 -Ptests.multiplier=2 -Ptests.vectorsize=128 -Ptests.iters=50 -Ptests.seed=ACE84AE4C131D76C

@github-actions
Copy link
Contributor

github-actions bot commented Jun 26, 2025

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

@thecoop thecoop requested a review from dweiss June 26, 2025 09:25
@thecoop thecoop requested a review from ChrisHegarty July 9, 2025 14:19
@thecoop thecoop mentioned this pull request Jul 10, 2025
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2025

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Jul 25, 2025
@thecoop thecoop mentioned this pull request Aug 7, 2025
Merged
ChrisHegarty
ChrisHegarty approved these changes Aug 8, 2025
View reviewed changes
Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2025

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

@github-actions github-actions bot added this to the 11.0.0 milestone Aug 8, 2025
@ChrisHegarty ChrisHegarty mentioned this pull request Aug 8, 2025
Merged
msokolov
msokolov approved these changes Aug 8, 2025
View reviewed changes
Copy link
Contributor

@msokolov msokolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm still kind of wary about the multiple abstractions: what impact will the hint actually have? I guess it depends on how the Directory chooses to interpret it? But this seems like progress!

@thecoop thecoop merged commit a43c02d into apache:main Aug 8, 2025
8 checks passed
@thecoop thecoop deleted the reapply-readadvice-iocontext branch August 8, 2025 14:54
jpountz pushed a commit to shubhamvishu/lucene that referenced this pull request Aug 10, 2025
@thecoop @jpountz
Reapply "Update the IOContext rather than the ReadAdvice on IndexInput (
6f24cdd 
apache#14702)" (apache#14844)
This was referenced Aug 11, 2025
Merged
Merged
msokolov pushed a commit to msokolov/lucene that referenced this pull request Aug 14, 2025
@thecoop
Reapply "Update the IOContext rather than the ReadAdvice on IndexInput (
6207ca2 
apache#14702)" (apache#14844)

fixes apache#15071
@msokolov msokolov mentioned this pull request Aug 14, 2025
Merged
msokolov added a commit that referenced this pull request Aug 14, 2025
@msokolov @thecoop
Reapply "Update the IOContext rather than the ReadAdvice on IndexInput (
9135700 
#14702)" (#14844) (#15073)

fixes #15071

Co-authored-by: Simon Cooper <simon.cooper@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpountz jpountz jpountz left review comments

@msokolov msokolov msokolov approved these changes

@ChrisHegarty ChrisHegarty ChrisHegarty approved these changes

@dweiss dweiss Awaiting requested review from dweiss

Assignees

No one assigned

Labels

module:core/codecs module:core/store module:test-framework Stale

Projects

None yet

Milestone

11.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@thecoop @dweiss @jpountz @msokolov @ChrisHegarty