Add hash of key/value checks when paranoid_file_checks=true #7134
Conversation
db/compaction/compaction_job.cc
Outdated
| curr->paranoid_hash = Hash64(key.data(), key.size(), curr->paranoid_hash); | ||
| curr->paranoid_hash = | ||
| Hash64(value.data(), value.size(), curr->paranoid_hash); | ||
| } |
There was a problem hiding this comment.
Can we make this logic (Add() and paranoid check) into sub_compact->builder, so that in case in the future we add K/Vs in another place, we won't miss this paranoid check.
Btw, I remember there is another place we call sub_compact->builder->Add(), do we need to cover that path too?
There was a problem hiding this comment.
Are you suggesting TableBuilder::Add do this calculation? That could be done, but then all of the implementations of TableBuilder would need to perform the calculation (or pass it to a higher layer).
There are other places Builder::Add is called, to add things like range deletes. Should those also be covered by this case?
There was a problem hiding this comment.
Sorry I meant to move to sub_compact. I didn't mean to move into Builder::Add(). I hope this hash check is independent of table builder implementation.
db/corruption_test.cc
Outdated
| Build(10, 2); | ||
| mock->SetCorruptionMode(mode); | ||
| DBImpl* dbi = static_cast_with_check<DBImpl>(db_); | ||
| dbi->TEST_FlushMemTable(); |
There was a problem hiding this comment.
Maybe I missed something but how do we make sure flush doesn't hit the corruption but the compaction range below hit it?
There was a problem hiding this comment.
Re-arranged the test to set the corruption after the flush but before the compaction.
When paraoid_files_checks=true, a rolling key-value hash is generated and compared to what is written to the file. If the values do not match, the SST file is rejected.
mrambacher
force-pushed
the
ParanoidChecks
branch
from
f9a3d9e to
68702ea
Compare
|
Rebased to master and squashed the commits. Updated the comment in advanced_options.h |
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -27,6 +27,154 @@ stl_wrappers::KVMap MakeMockFile( | |||
| return stl_wrappers::KVMap(l, stl_wrappers::LessOfComparator(&icmp_)); | |||
| } | |||
|
|
|||
| class MockTableReader : public TableReader { | |||
There was a problem hiding this comment.
Warning:
Encapsulate structs and classes defined in cpp files with anonymous namespace
Struct and class definitions should either be defined in a header file, declared in a header file with the same base name and path as the definition file, or in an anonymous namespace to prevent possible shadowing of other classes.
There was a problem hiding this comment.
There are similar calls in other new classes in the file.
| } | ||
|
|
||
| // REQUIRES: Either Finish() or Abandon() has been called. | ||
| ~MockTableBuilder() {} |
There was a problem hiding this comment.
nternal_repo_rocksdb/repo/table/mock_table.cc:61:3: warning: '~MockTableReader' overrides a destructor but is not marked 'override'
There are similar calls in other new classes in the file.
|
This pull request has been merged in d44cbc5. |
…#7134) Summary: When paraoid_files_checks=true, a rolling key-value hash is generated and compared to what is written to the file. If the values do not match, the SST file is rejected. Code put in place for the check for both flush and compaction jobs. Corresponding test added to corruption_test. Pull Request resolved: facebook#7134 Reviewed By: cheng-chang Differential Revision: D22646149 fbshipit-source-id: 8fde1984a1a11edd3bd82a413acffc5ea7aa683f
When paraoid_files_checks=true, a rolling key-value hash is generated and compared to what is written to the file. If the values do not match, the SST file is rejected.
Code put in place for the check for both flush and compaction jobs. Corresponding test added to corruption_test.