db_stress support tracking historical values #8960
Conversation
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
ajkr
force-pushed
the
dbstress-expected-state-saveatandafter
branch
from
2d02085
to
4741881
Compare
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks @ajkr for the PR. Left a few initial minor comments before taking closer look.
db_stress_tool/db_stress_gflags.cc
Outdated
| @@ -789,7 +790,10 @@ DEFINE_int32(get_property_one_in, 1000, | |||
|
|
|||
| DEFINE_bool(sync_fault_injection, false, | |||
| "If true, FaultInjectionTestFS will be used for write operations, " | |||
| " and unsynced data in DB will lost after crash."); | |||
| "and unsynced data in DB will lost after crash. In such a case we " | |||
| "track DB changes in a trace file in --expected_values_dir for " | |||
There was a problem hiding this comment.
Maybe give an example of the name of a trace file? For example, it can be reworded a bit like:
track DB changes in a trace file (*.trace)
There was a problem hiding this comment.
What is the expectation if expected_values_dir is empty but sync_fault_injection is set to true? The doc for sync_fault_injection seems to imply that "in such a case, we will need expected_values_dir.
If so, can we add a check in command line arguments parsing and processing, so that we make sure user sets expected_values_dir if they set sync_fault_injection to true?
There was a problem hiding this comment.
Maybe give an example of the name of a trace file?
Done.
There was a problem hiding this comment.
What is the expectation if expected_values_dir is empty but sync_fault_injection is set to true?
Empty expected_values_dir means no verification at startup, and verification at shutdown based only on the operations performed by that particular db_stress run. Combining it with sync_fault_injection=true would not bring additional coverage (compared to sync_fault_injection=false) because sync_fault_injection only causes unsynced data to be dropped at process end, which is after all verifications when expected_values_dir is empty.
Since I believe the combination brings no additional coverage, it is currently not supported. The error looks like this:
$ ./db_stress --expected_values_dir="" --sync_fault_injection=true
...
Error enabling history tracing: Not implemented:
db_stress_tool/expected_state.cc
Outdated
| @@ -239,6 +386,10 @@ Status AnonExpectedStateManager::Open() { | |||
| return latest_->Open(true /* create */); | |||
| } | |||
|
|
|||
| Status AnonExpectedStateManager::SaveAtAndAfter(DB* /* db */) { | |||
There was a problem hiding this comment.
Nit: can this be inlined just to save some typing?
| std::string temp_path = GetTempPathForFilename(kLatestFilename); | ||
| Status s = Env::Default()->FileExists(temp_path); | ||
| #ifndef ROCKSDB_LITE | ||
| Status FileExpectedStateManager::SaveAtAndAfter(DB* db) { |
There was a problem hiding this comment.
Is SaveAtAndAfter only going to be called on DB open, or eventually also anytime the WAL is synced?
There was a problem hiding this comment.
I would like to call it anytime the WAL is synced. However there are some difficulties with parallel writers mutating the "*.state" file while it's being copied, for example. We would need to figure out the locking to make it possible.
ajkr
force-pushed
the
dbstress-expected-state-saveatandafter
branch
from
4741881
to
8242f86
Compare
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
2 similar comments
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
ajkr
force-pushed
the
dbstress-expected-state-saveatandafter
branch
from
ecacfa3
to
db46d2e
Compare
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
db_stress_tool/expected_state.cc
Outdated
| } | ||
| } | ||
| if (s.ok()) { | ||
| if (old_saved_seqno != kMaxSequenceNumber && |
There was a problem hiding this comment.
Nit: duplicate if could be simplified
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
db_stress_tool/expected_state.cc
Outdated
| } | ||
| } | ||
| if (s.ok()) { | ||
| if (old_saved_seqno != kMaxSequenceNumber && |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
When
--sync_fault_injectionis set, this PR takes a snapshot of the expected values and starts an operation trace when the DB is opened. These files are stored in--expected_values_dir. They will be used for recovering the expected state of the DB following a crash where a suffix of unsynced operations are allowed to be lost.Test Plan: injected crashed at various points in
FileExpectedStateManagerand verified the next run recovers the state/trace file with highest seqno and removes all older/temporary files. Note we don't use sync_fault_injection in CI crash tests yet.