Fix missing cases of corruption retries #13122
Conversation
| // is allocated with max_open_files - 10 as capacity. So override | ||
| // max_open_files to 11 so table cache capacity will become 1. This will | ||
| // prevent file open during DB open and force the file to be opened | ||
| // during MultiGet |
| } | ||
| if (s.ok()) { | ||
| s = ValidityCheck(); | ||
| } |
There was a problem hiding this comment.
nit: if (s.ok()) { return s; } after this validity check would be more readable to me then setting retry = false at the else in line 349. Or you could explicitly return on line 349.
| s = ValidityCheck(); | ||
| } | ||
| if (!s.ok()) { | ||
| if ((s.IsCorruption() || s.IsInvalidArgument()) && !retry && |
There was a problem hiding this comment.
Question for my own learning. In what scenario that we'd get s.IsInvalidArgument() here and retry with kVerifyAndReconstructRead would succeed?
There was a problem hiding this comment.
Any syntax errors during parsing are being considered as Status::InvalidArgument(). The syntax error could be due to corruption, and can potentially be corrected by retrying.
db/db_io_failure_test.cc
Outdated
| return s; | ||
| } | ||
|
|
||
| // This means the next read after injecting corruption was not |
There was a problem hiding this comment.
wondering if the comment was cut
db/db_io_failure_test.cc
Outdated
| ss << std::setw(3) << 100 * sst + key; | ||
| ASSERT_OK(Put("key" + ss.str(), "val" + ss.str())); | ||
| } | ||
| Flush(); |
| } | ||
| Flush(); | ||
| } | ||
| Close(); |
There was a problem hiding this comment.
Looks like DBTestBase::Close() does not return a Status.
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@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 ee25861. |
Summary: This PR fixes a few cases where RocksDB was not retrying checksum failure/corruption of file reads with the `verify_and_reconstruct_read` IO option. After fixing these cases, we can almost always successfully open the DB and execute reads even if we see transient corruptions, provided the `FileSystem` supports the `verify_and_reconstruct_read` option. The specific cases fixed in this PR are - 1. CURRENT file 2. IDENTITY file 3. OPTIONS file 4. SST footer Pull Request resolved: #13122 Test Plan: Unit test in `db_io_failure_test.cc` that injects corruption at various stages of DB open and reads Reviewed By: jaykorean Differential Revision: D65617982 Pulled By: anand1976 fbshipit-source-id: 4324b88cc7eee5501ab5df20ef7a95bb12ed3ea7
This PR fixes a few cases where RocksDB was not retrying checksum failure/corruption of file reads with the
verify_and_reconstruct_readIO option. After fixing these cases, we can almost always successfully open the DB and execute reads even if we see transient corruptions, provided theFileSystemsupports theverify_and_reconstruct_readoption. The specific cases fixed in this PR are -Test plan:
Unit test in
db_io_failure_test.ccthat injects corruption at various stages of DB open and reads