-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Verify explicitly synced data is recoverable #12152
Comments
In case it's not already obvious, for WAL disabled case, we will only consider with |
hi, ajkr@ and hx235@. |
@jacobxli-xx Thank you, it is assigned now. |
@jacobxli-xx Here are some questions I had about what you are planning to do:
If any answers are no, we'll probably want to know the reason and eventually the plan to make a guess of whether we'd be on board with it. |
Hey, Andrew. |
@jacobxli-xx If you are considering storing and m-mapping map<std::string, uint64_t> cf_flushed_seqno, I wonder why can't we just store and mmap the maximum of all the seqnos in the map? |
@hx235 The m-mapping map<std::string, uint64_t> cf_flushed_seqno stores the cf name and the corresponding maximum flushed seqno of the specific cf. Why we do not only store the maximum of all the seqnos is because when we do explicit flush on specific cf, it's not guaranteed that all cfs will be flushed, so the maximum of all seqno is not guaranteed to be equal to the maximum seqno of the cf we flushed. Although our test cases are limited to 1 cf, we use this map for forward compatibility as it can be used in multiple cfs cases. |
It is guaranteed in the case where it is needed (atomic_flush=true) - rocksdb/db_stress_tool/db_stress_test_base.cc Lines 2266 to 2268 in 3ef9092
It is not guaranteed when WAL is enabled and atomic_flush is disabled, but there it is not needed because WAL ensures consistent recovery across column families. WAL-disabled, atomic_flush-disabled may be worth testing at some future point but that is still an open topic to discuss. My thoughts are here - #11841. Another case that may be worth testing at some point is WAL-disabled, atomic_flush-enabled, and manual flush triggered on a subset of column families (what you suggested we are already doing). Still, it's not something we decided to do yet, so adding extra complexity assuming we will do it feels premature |
The thoughts on WAL-disabled, atomic_flush-disabled cases in #11841 sounds reasonable to me. Using only one seqno should be fine. Will modify the implementation a bit accordingly. |
Hi, an update on the status. Planning on sending out the PR by Apr 10th. |
This issue is a next step mentioned in my blog post (https://rocksdb.org/blog/2022/10/05/lost-buffered-write-recovery.html):
An untested guarantee is that RocksDB recovers all writes that the user explicitly flushed out of the buffers lost in the crash. We may recover more writes than these due to internal flushing of buffers, but never less. Our test oracle needs to be further extended to track the lower bound on the sequence number that is expected to survive a crash.
The first step is to handle the single column family, WAL disabled case. We could use either seqno for tracking or a new write counter internal to the test oracle. We should update the expected minimum value of that counter for (1) explicit flushes, and (2) OnFlushCompleted() events. We should verify the recovery reaches at least the expected minimum value.
The text was updated successfully, but these errors were encountered: