Track WAL in MANIFEST: add option track_and_verify_wals_in_manifest #7275

ghost · 2020-08-18T01:39:05Z

This option determines whether WALs will be tracked in MANIFEST and verified on recovery.

Test Plan:
db_options_test
options_test

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

riversand963 · 2020-08-18T18:34:19Z

options/db_options.h

@@ -22,6 +22,7 @@ struct ImmutableDBOptions {
  bool create_missing_column_families;
  bool error_if_exists;
  bool paranoid_checks;
+  bool check_wal;


NIt: per our team discussion, maybe it's better to name it should_check_wal? cc @pdillinger

On further reflection (re naming in #7214), I am more critical of re-using verb member variable names in classes (as accessors) than having them in structs to begin with. This struct already has a lot of verb names. ¯_(ツ)_/¯

But I do think it's worth thinking about a better name. verify_wals? verify_wals_with_manifest?

I think check_wal delivers the meaning, there are other "checks", such as "paranoid_checks", "check_sst_file_size", etc.

@riversand963 @pdillinger does track_and_verify_wals_in_manifest sound good to you?

pdillinger · 2020-08-18T19:52:29Z

include/rocksdb/options.h

+  // No matter whether this is true or false, the WAL information are always
+  // tracked by MANIFEST.
+  // Default: false
+  bool check_wal = false;


Why wouldn't this be true by default if we always pay the ongoing cost of tracking, and the only check is during recovery?

When / why would someone want to use false?

If the user does not even sync WALs, then the user might skip checking the WALs since there is no guarantee of WAL's existence or completeness on recovery.

pdillinger · 2020-08-18T19:56:11Z

include/rocksdb/options.h

@@ -374,6 +374,14 @@ struct DBOptions {
  // Default: true
  bool paranoid_checks = true;

+  // If true, check on-disk WALs against WAL information stored in MANIFEST
+  // during recovery.


"check" is not very descriptive to me. How about something like "If true, fail recovery if there is an inconsistency between WAL information in MANIFEST and actual WALs on disk"?

What sort of corruption might this catch that's not otherwise caught? (Don't we always want to report corruption?)

Also, do we quietly ignore extra/untracked WALs or fail in that case?

missing or corrupted WAL will be reported

It's still not clear to me how extra/untracked WALs will be handled. Can a DB switch between using track_and_verify_wals_in_manifest=true and false?

Yes, switches between true and false can happen. The verification will only verify those tracked WALs.

ghost · 2020-10-06T03:58:25Z

@riversand963 actually, I think Peter's suggestion makes sense. Since we always track WAL in MANIFEST, it doesn't make too much sense to disable checking WALs during recovery. This PR may be unnecessary. What's your thought?

riversand963 · 2020-10-06T17:57:50Z

@riversand963 actually, I think Peter's suggestion makes sense. Since we always track WAL in MANIFEST, it doesn't make too much sense to disable checking WALs during recovery. This PR may be unnecessary. What's your thought?

It sounds a good idea to always check WAL.
Another question, would some users want to disable WAL tracking sometimes for performance considerations? If so, the wal verification logic should keep this in mind.

ghost · 2020-10-07T21:00:01Z

After offline discussion, we'll keep this check_wal option, if it's false, then WALs are not tracked in MANIFEST and won't be checked on recovery.

facebook-github-bot · 2020-10-07T23:42:55Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-07T23:47:20Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-07T23:50:53Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-07T23:53:31Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2020-10-08T00:09:15Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-08T03:35:12Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-08T23:53:08Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pdillinger

Even though the new name is kind of long, I think it's clearer and therefore better.

I don't know this area well enough to endorse or reject the overall strategy, but my previous concerns are sufficiently addressed. Some additional comments / clarification requested.

pdillinger · 2020-10-09T20:51:05Z

include/rocksdb/options.h

@@ -374,6 +374,14 @@ struct DBOptions {
  // Default: true
  bool paranoid_checks = true;

+  // If true, check on-disk WALs against WAL information stored in MANIFEST
+  // during recovery.


It's still not clear to me how extra/untracked WALs will be handled. Can a DB switch between using track_and_verify_wals_in_manifest=true and false?

pdillinger · 2020-10-09T20:51:09Z

include/rocksdb/options.h

+  // during recovery.
+  //
+  // Default: false
+  bool track_and_verify_wals_in_manifest = false;


If this option is not currently connected to anything (we don't have diff stacks unfortunately), please drop in something like

// FIXME(cheng): This option is part of a work in progress and does not yet work

in case through some mixup or unexpected absence from work, a release only has some of your changes

facebook-github-bot · 2020-10-09T21:09:55Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-09T21:29:40Z

@cheng-chang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-10-10T00:38:41Z

@Cheng-Chang merged this pull request in 12b78e4.

…acebook#7275) Summary: This option determines whether WALs will be tracked in MANIFEST and verified on recovery. Pull Request resolved: facebook#7275 Test Plan: db_options_test options_test Reviewed By: pdillinger Differential Revision: D23181418 Pulled By: cheng-chang fbshipit-source-id: 5dd1cdc166f3dfc1c93c094df4a2f7734e3b4547

Cheng Chang added 9 commits February 24, 2020 09:10

Update release version to 6.8

8be1e81

Merge branch 'master' of https://github.com/facebook/rocksdb

90913ec

Merge branch 'master' of https://github.com/facebook/rocksdb

2bac9fd

Merge branch 'master' of https://github.com/facebook/rocksdb

b672da1

Merge branch 'master' of https://github.com/facebook/rocksdb

15a1e94

Merge branch 'master' of https://github.com/facebook/rocksdb

2fe6ba7

add option check_wal

2160884

add check_wal to random options

661faf1

add more tests

8eb6a18

facebook-github-bot added the CLA Signed label Aug 18, 2020

ghost requested review from ajkr, riversand963 and ltamasi August 18, 2020 01:39

facebook-github-bot reviewed Aug 18, 2020

View reviewed changes

riversand963 reviewed Aug 18, 2020

View reviewed changes

pdillinger reviewed Aug 18, 2020

View reviewed changes

Cheng Chang added 5 commits September 14, 2020 16:21

Merge branch 'master' of https://github.com/facebook/rocksdb

95b9bdc

Merge branch 'master' of https://github.com/facebook/rocksdb

b966b89

Merge branch 'master' of https://github.com/facebook/rocksdb

fa820eb

Merge branch 'master' of https://github.com/facebook/rocksdb

f8e9025

Merge branch 'master' of https://github.com/facebook/rocksdb

77861da

ghost removed request for ajkr and ltamasi October 6, 2020 03:58

Merge branch 'master' of https://github.com/facebook/rocksdb

93ee759

ghost changed the title ~~Track WAL in MANIFEST: add option check_wal~~ Track WAL in MANIFEST: add option track_and_verify_wals_in_manifest Oct 7, 2020

Cheng Chang added 2 commits October 7, 2020 16:36

rename check_wal

ccb6ebf

update comment

2cec5b5

Cheng Chang added 2 commits October 7, 2020 16:46

Merge branch 'master' into option_check_wal

6606e96

Update db_options.h

6ad5c24

facebook-github-bot reviewed Oct 7, 2020

View reviewed changes

Update db_options.cc

131f305

Update db_options_test.cc

44138e2

update test

cabf400

facebook-github-bot reviewed Oct 8, 2020

View reviewed changes

Update db_options_test.cc

41b1209

facebook-github-bot reviewed Oct 8, 2020

View reviewed changes

Update db_options.cc

9e7fc8c

facebook-github-bot reviewed Oct 9, 2020

View reviewed changes

pdillinger approved these changes Oct 9, 2020

View reviewed changes

facebook-github-bot reviewed Oct 9, 2020

View reviewed changes

leave a todo

8a033ed

facebook-github-bot reviewed Oct 9, 2020

View reviewed changes

facebook-github-bot closed this in 12b78e4 Oct 9, 2020

facebook-github-bot added the Merged label Oct 10, 2020

Track WAL in MANIFEST: add option track_and_verify_wals_in_manifest #7275

Track WAL in MANIFEST: add option track_and_verify_wals_in_manifest #7275

Conversation

ghost commented Aug 18, 2020 • edited by ghost Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 7, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 6, 2020

riversand963 commented Oct 6, 2020

ghost commented Oct 7, 2020

facebook-github-bot commented Oct 7, 2020

facebook-github-bot commented Oct 7, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 7, 2020

facebook-github-bot commented Oct 7, 2020

facebook-github-bot commented Oct 8, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 8, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 8, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

pdillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 9, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 9, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 10, 2020

ghost commented Aug 18, 2020 •

edited by ghost

Loading

ghost Oct 7, 2020 •

edited by ghost

Loading