Encode min_log_number_to_keep and delete_wals_before in one version edit #9766
Conversation
riversand963
force-pushed
the
wal-del-atomic
branch
from
a327daa to
19c9664
Compare
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
Thanks for the fix @riversand963 ! LGTM in general, I just have one question
| if (has_min_log_number_to_keep_) { | ||
| PutVarint32Varint64(dst, kMinLogNumberToKeep, min_log_number_to_keep_); | ||
| } |
There was a problem hiding this comment.
Just wondering if this is safe... IIUC the reason this wasn't encoded earlier had to do with the MANIFEST not being forward compatible (see the kMinLogNumberToKeepHack custom tag of kNewFile4)
There was a problem hiding this comment.
Right, but as early as 5.14, RocksDB is able to understand kMinLogNumberTokeep (https://github.com/facebook/rocksdb/blob/5.14.fb/db/version_edit.cc#L360), so I guess it's OK? It means user cannot use earlier versions than 5.14 to open a DB generated with versions newer than 7.0 (if we backport).
Summary: min_log_number_to_keep denotes that the WALs whose numbers are below this value will be deleted by RocksDB. delete_wals_before will be used by RocksDB if track_and_verify_wals_in_manifest is set to true. During recovery, RocksDB uses the info encoded in delete_wals_before to reconstruct its knowledge about what WALs to expect existing. If these two tags are not encoded in the same VersionEdit, then it's possible for min_log_number_to_keep=100 to exist, but delete_wals_before=100 to be lost due to power failure. Subsequent recovery will delete 99.log. If the db crashes again, the following recovery will expect to see 99.log since there is no delete_wals_before=100 in the MANIFEST, but the WAL is already deleted. Test Plan: First of all, make check. Second, format compatibility. SHORT_TEST=1 ./tools/check_format_compatible.sh
riversand963
force-pushed
the
wal-del-atomic
branch
from
5d09162 to
d93833a
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Thanks @ltamasi for the review! |
…dit (#9766) Summary: min_log_number_to_keep denotes that the WALs whose numbers are below this value **will** be deleted by RocksDB. delete_wals_before will be used by RocksDB if track_and_verify_wals_in_manifest is set to true. During recovery, RocksDB uses the info encoded in delete_wals_before to reconstruct its knowledge about what WALs to expect existing. If these two tags are not encoded in the same VersionEdit, then it's possible for min_log_number_to_keep=100 to exist, but delete_wals_before=100 to be lost due to power failure. Subsequent recovery will delete 99.log. If the db crashes again, the following recovery will expect to see 99.log since there is no delete_wals_before=100 in the MANIFEST, but the WAL is already deleted. Pull Request resolved: #9766 Test Plan: First of all, make check. Second, format compatibility. SHORT_TEST=1 ./tools/check_format_compatible.sh Reviewed By: ltamasi Differential Revision: D35203623 Pulled By: riversand963 fbshipit-source-id: 45623fc4b4b50d299d5e0f9559a3a4c5e9522c8f
Summary:
min_log_number_to_keep denotes that the WALs whose numbers are below
this value will be deleted by RocksDB.
delete_wals_before will be used by RocksDB if
track_and_verify_wals_in_manifest is set to true. During recovery,
RocksDB uses the info encoded in delete_wals_before to reconstruct its
knowledge about what WALs to expect existing.
If these two tags are not encoded in the same VersionEdit, then it's
possible for min_log_number_to_keep=100 to exist, but
delete_wals_before=100 to be lost due to power failure. Subsequent
recovery will delete 99.log. If the db crashes again, the following
recovery will expect to see 99.log since there is no
delete_wals_before=100 in the MANIFEST, but the WAL is already deleted.
Test Plan:
First of all, make check.
Second, format compatibility.
SHORT_TEST=1 ./tools/check_format_compatible.sh