Fix a failure to propagate ReadOptions #12757
Conversation
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
I'm good with the partitioned_index_reader fix but want to discuss more on the EnforceReadOpts and thread-local context. So feel free to separate this into two PRs to unblock stress test. For For |
pdillinger
force-pushed
the
read_options_propagate1
branch
from
77ed61b
to
96badde
Compare
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
OK, PR is split (next one with EnforceReadOpts to be created) |
Summary: ... in Index and CompressionDict readers (Filters in another PR). no_io and verify_checksums should be inferred from ReadOptions rather than specified redundantly. Fixes incomplete propagation of ReadOptions in UncompressionDictReader::GetOrReadUncompressionDictionar so is technically a functional change. (Related to facebook#12757) Also there was hardcoded no verify_checksums in DumpTable, but only for UncompressionDict, which doesn't make sense. Now using consistent ReadOptions and verify_checksum can be controlled for more reads together. Test Plan: existing tests
|
@pdillinger merged this pull request in d64eac2. |
Summary: Follow-up from facebook#12757. New infrastructure to DEBUG builds to catch certain ReadOptions being ignored in a context where they should be in effect. This currently only applies to checking that no IO happens when read_tier == kBlockCacheTier. The check is in effect for unit tests and for stress/crash tests. Specifically, an `EnforceReadOpts` object on the stack establishes a thread-local context under which we assert no IOs are performed if the provided ReadOptions said it should be forbidden. Test Plan: Reports failure before production code fix in facebook#12757
Summary: ... in Index and CompressionDict readers (Filters in another PR). no_io and verify_checksums should be inferred from ReadOptions rather than specified redundantly. Fixes incomplete propagation of ReadOptions in UncompressionDictReader::GetOrReadUncompressionDictionar so is technically a functional change. (Related to #12757) Also there was hardcoded no verify_checksums in DumpTable, but only for UncompressionDict, which doesn't make sense. Now using consistent ReadOptions and verify_checksum can be controlled for more reads together. Pull Request resolved: #12761 Test Plan: existing tests Reviewed By: hx235 Differential Revision: D58450392 Pulled By: pdillinger fbshipit-source-id: 0faed22832d664cb3b04a4c03ee77119977c200b
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.
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.
EnforceReadOptsestablishes 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.