Skip to content

Change default ReadAdvice from RANDOM to NORMAL#15040

Merged
ChrisHegarty merged 3 commits intoapache:mainfrom
ChrisHegarty:default_read_advice
Aug 8, 2025
Merged

Change default ReadAdvice from RANDOM to NORMAL#15040
ChrisHegarty merged 3 commits intoapache:mainfrom
ChrisHegarty:default_read_advice

Conversation

@ChrisHegarty
Copy link
Contributor

@ChrisHegarty ChrisHegarty commented Aug 8, 2025

This commit changes the default ReadAdvice from RANDOM to NORMAL.

After this change MMapDirectory will not set any specific read advice out-of-the-box. Rather there is a straightforward way to enable either previous behaviour ( in 10.2 ), by setting either:

  1. dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT), or
  2. setting the system property -Dorg.apache.lucene.store.defaultReadAdvice=RANDOM.

closes #14408

@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 10.3.0 milestone Aug 8, 2025
@msokolov
Copy link
Contributor

msokolov commented Aug 8, 2025

This seems like good progress. I'm also unsure about Lucene99FlatVectorsReader.finishMerge() where we call
this.vectorData.updateReadAdvice(ReadAdvice.RANDOM); and related code in that class that was added in #13985 -- going back to read that now

@msokolov msokolov self-requested a review August 8, 2025 13:35
@msokolov
Copy link
Contributor

msokolov commented Aug 8, 2025

Yeah, I worry that this now conflicts with what we did in the vectors reader, since the default will be normal, but after merging any vectors segments we will switch back to random?

@ChrisHegarty
Copy link
Contributor Author

@msokolov I agree - there is more needed here. Let's get #14844 in first, then we can revaluate if/how to do this properly in this PR.

@ChrisHegarty ChrisHegarty requested a review from thecoop August 8, 2025 16:05
@ChrisHegarty ChrisHegarty merged commit cefc6c7 into apache:main Aug 8, 2025
8 checks passed
jpountz pushed a commit to shubhamvishu/lucene that referenced this pull request Aug 10, 2025
This commit changes the default ReadAdvice from RANDOM to NORMAL.

After this change MMapDirectory will not set any specific read advice out-of-the-box. Rather there is a straightforward way to enable either previous behaviour ( in 10.2 ), by setting either:
1. dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT), or
2. setting the system property -Dorg.apache.lucene.store.defaultReadAdvice=RANDOM.

closes apache#14408
ChrisHegarty added a commit that referenced this pull request Aug 11, 2025
This commit changes the default ReadAdvice from RANDOM to NORMAL.

After this change MMapDirectory will not set any specific read advice out-of-the-box. Rather there is a straightforward way to enable either previous behaviour ( in 10.2 ), by setting either:
1. dir.setReadAdvice(MMapDirectory.ADVISE_BY_CONTEXT), or
2. setting the system property -Dorg.apache.lucene.store.defaultReadAdvice=RANDOM.

closes #14408
@ChrisHegarty ChrisHegarty deleted the default_read_advice branch August 11, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Examine the affects of MADV_RANDOM when MGLRU is enabled in Linux kernel

3 participants