Ensure VerifyFileChecksums reads don't exceed readahead_size #11328
Conversation
| } | ||
| ASSERT_OK(Flush()); | ||
|
|
||
| ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( |
There was a problem hiding this comment.
The test should fail if somehow the callback stops being called at all. Perhaps declare last_read outside the callback and ASSERT_TRUE at the end of the test.
There was a problem hiding this comment.
I suppose I can count the bytes read and assert outside the callback that its equal to the file size.
db/db_basic_test.cc
Outdated
| ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( | ||
| "GenerateOneFileChecksum::Chunk:0", [&](void* /*arg*/) { | ||
| static bool last_read = false; | ||
| if (env_->random_read_bytes_counter_.load() & (256 * 1024 - 1)) { |
There was a problem hiding this comment.
This seems to be allowing only one read that is not a multiple of 256KB. I'm not sure why that's necessary and sufficient.
There was a problem hiding this comment.
The expected pattern is a bunch of aligned reads, followed by 0 or 1 unaligned read at the tail of the file.
anand1976
force-pushed
the
cksum_readahead
branch
from
cb23f71 to
cd62ce5
Compare
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
anand1976
force-pushed
the
cksum_readahead
branch
from
cd62ce5 to
7693b0f
Compare
|
@anand1976 has updated the pull request. You must reimport the pull request before landing. |
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@anand1976 merged this pull request in 0623c5b. |
Summary: VerifyFileChecksums currently interprets the readahead_size as a payload of readahead_size for calculating the checksum, plus a prefetch of an additional readahead_size. Hence each read is readahead_size * 2. This change treats it as chunks of readahead_size for checksum calculation. Pull Request resolved: #11328 Test Plan: Add a unit test Reviewed By: pdillinger Differential Revision: D44718781 Pulled By: anand1976 fbshipit-source-id: 79bae1ebaa27de2a13bc86f5910bf09356936e63
Summary: I suspect the issue called out in facebook#9357 was fixed in facebook#11328 Test Plan: `make blackbox_crash_test` for hours
VerifyFileChecksums currently interprets the readahead_size as a payload of readahead_size for calculating the checksum, plus a prefetch of an additional readahead_size. Hence each read is readahead_size * 2. This change treats it as chunks of readahead_size for checksum calculation.
Test plan:
Add a unit test