Skip to content

Make the default ReadAdvice configurable by sysprop #13264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

uschindler
Copy link
Contributor

Followup after #13244: This makes the default ReadAdvice for index files configurable via system property org.apache.lucene.store.defaultReadAdvice.

I moved the corresponding constant to the oal.util.Constants class because this makes reading and error reporting easier as @rmuir and I added some framework to safely support security manager and map those variables (we used this for vectorization).

It would be good to move all reading of system properties to that class so we do not have the splattered AccessControllers everywhere.

@uschindler uschindler added this to the 10.0.0 milestone Apr 4, 2024
@uschindler uschindler requested a review from jpountz April 4, 2024 15:50
@uschindler uschindler self-assigned this Apr 4, 2024
@uschindler
Copy link
Contributor Author

P.S.: I moved the lookup cache to a new constant name and placed it next to the withReadAdvice() method.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

This makes sense to me. At first I wondered if we needed this since it's possible to create a FilterDirectory that calls ioContext.withReadAdvice(ReadAdvice.NORMAL) on Directory#openIndexInput or something along these lines. But actually this wouldn't work as well, as this would override explicit decisions made from the code, which is probably not what we want.

@uschindler
Copy link
Contributor Author

uschindler commented Apr 4, 2024

This makes sense to me. At first I wondered if we needed this since it's possible to create a FilterDirectory that calls ioContext.withReadAdvice(ReadAdvice.NORMAL) on Directory#openIndexInput or something along these lines. But actually this wouldn't work as well, as this would override explicit decisions made from the code, which is probably not what we want.

That was exactly my problem with making the default hardcoded.

I thought about making it configurable "per directory", but this is too much changes for little benefit. As this is mostly machine specific, a system property that can be passed at command line was my favourite.

In addition, we should still pass iocontext.withReadAdvice(ReadAdvice.RANDOM) in our codecs when opening index files, where we know for sure that the access will be fully random (like HNSW), so the other PR you implemented was still fine.

So the default specified by system property is only used for cases where no explicit read advice is given at all .

@uschindler uschindler merged commit f7db975 into apache:main Apr 4, 2024
@uschindler uschindler deleted the dev/default_read_advice branch April 4, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants