Open hx235 opened 1 week ago
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
irrelevant error failure
Discussion with @cbi42 also includes leaving the PENDING STATE of expected value upon injected error. Thinking about it
@hx235 has updated the pull request. You must reimport the pull request before landing.
Would it work if we just keep the expected state at pending state? Ideally only when injected error is from writing to MANIFEST. If the failure is from writing to WAL, we can just treat it as failed.
I decided to treat write-to-WAL failure the same as the manifest case for two reasons: they both triggers auto-recovery and it's hard to do so thoroughly now when SyncDir fails for WAL with metadata write error injection.
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Irrelevant crash test failures
@hx235 has updated the pull request. You must reimport the pull request before landing.
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
After further discussion with @cbi42, for write failed after WAL write (e.g, ApplyWALToManifest failure), its expected state is left pending. For other writes, we either rollback or commit the expected state. To close those pending expected state, initial thought was to loop-write the same key till success. But that surfaced an internal bug not easy to fix (being discussed internally). So for now, this PR closes those expected state as part of the restart (VerifyOrSyncValue()
) naturally. Downside is up to n=number of crash test threads (default 32) keys are left pending state till next db restart (instead of till error recovery finishes during the same session) and won't accept any write.
Currently rebased onto https://github.com/facebook/rocksdb/pull/12860
Context/Summary: We discovered the following false positive in our crash test lately: (1) PUT() writes k/v to WAL but fails in
ApplyWALToManifest()
. The k/v is in the WAL so "effectively" in the DB (2) Current stress test logic will rollback the expected state of such k/v since PUT() fails (3) A concurrent read such as iterator scan can see the k/v in the DB but not the expected state so it complains.We decided to leave those expected state to be pending. [To fill in details about how we close the pending state].
Bonus: Now that I realized write to manifest can also fail the write which faces the similar problem as https://github.com/facebook/rocksdb/pull/12797, I decided to disable fault injection on user write per thread (instead of globally) when tracing is needed for prefix recovery; some refactory
Test:
iterator has key 0000000000000207000000000000012B0000000000000013, but expected state does not.
before this PR and passes after