-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
readahead options have inconsistent and unclear documentation #12756
Comments
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this issue
Jun 11, 2024
Summary: The crash test revealed a case in which the uncache functionality in ~BlockBasedTableReader could initiate an block read (IO), despite setting ReadOptions::read_tier = kBlockCacheTier. The root cause is a place in the code where many people have over time decided to opt-in propagating ReadOptions and no one took the initiative to propagate ReadOptions by default (opt out / override only as needed). The fix is in partitioned_index_reader.cc. Here, ReadOptions::readahead_size is opted-out to avoid churn in prefetch_test that is not clearly an improvement or regression. It's hard to tell given the poor state of relevant documentation facebook#12756. The affected unit test was added in facebook#10602. Test Plan: I have added some new infrastructure to DEBUG builds to catch this specific kind of violation in unit tests and in the stress/crash test. `EnforceReadOpts` establishes a thread-local context under which we assert no IOs are performed if ReadOptions said it should be forbidden. With this new checking, the Uncache unit test would catch the critical step toward a violation (inner ReadOptions allowing IO, even if no IO is actually performed), which is fixed with the production code change. Most of the diff is this new infrastructure.
facebook-github-bot
pushed a commit
that referenced
this issue
Jun 12, 2024
Summary: The crash test revealed a case in which the uncache functionality in ~BlockBasedTableReader could initiate an block read (IO), despite setting ReadOptions::read_tier = kBlockCacheTier. The root cause is a place in the code where many people have over time decided to opt-in propagating ReadOptions and no one took the initiative to propagate ReadOptions by default (opt out / override only as needed). The fix is in partitioned_index_reader.cc. Here, ReadOptions::readahead_size is opted-out to avoid churn in prefetch_test that is not clearly an improvement or regression. It's hard to tell given the poor state of relevant documentation #12756. The affected unit test was added in #10602. Pull Request resolved: #12757 Test Plan: (Now postponed to a follow-up diff) I have added some new infrastructure to DEBUG builds to catch this specific kind of violation in unit tests and in the stress/crash test. `EnforceReadOpts` establishes a thread-local context under which we assert no IOs are performed if ReadOptions said it should be forbidden. With this new checking, the Uncache unit test would catch the critical step toward a violation (inner ReadOptions allowing IO, even if no IO is actually performed), which is fixed with the production code change. Reviewed By: hx235 Differential Revision: D58421526 Pulled By: pdillinger fbshipit-source-id: 9e9917a0e320c78967e751bd887926a2ed231d37
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this issue
Jun 12, 2024
Summary: Not sure if this change is desirable given the unit test churn, but the unit test is not clear, in part because the meaning of readahead options is unclear (facebook#12756). Test Plan: existing tests (updated)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Both
adaptive_readahead
andreadahead_size
describe RocksDB behaviors, but not how that option (or others) affects that behavior. The comment foradaptive_readahead
refers to a non-existent optionmax_auto_readahead_size.
I don't see a relevant wiki page for a high-level coherent story. It is not clear howauto_readahead_size
,adaptive_readahead
, andreadahead_size
interact.The text was updated successfully, but these errors were encountered: