Fix corrupted wal number when predecessor wal corrupts + minor cleanup #13359

hx235 · 2025-01-31T20:05:16Z

Context/Summary:

02b4197 recently added the ability to detect WAL hole presents in the predecessor WAL. It forgot to update the corrupted wal number to point to the predecessor WAL in that corruption case. This PR fixed it.

As a bonus, this PR also (1) fixed the FragmentBufferedReader() constructor API to expose less parameters as they are never explicitly passed in in the codebase (2) a INFO log wording (3) a parameter naming typo (4) the reporter naming

Test:

Manual printing to ensure the corrupted wal number is set to the right number
Existing UTs

facebook-github-bot · 2025-02-03T20:00:24Z

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jowlyzhang

Sorry for the delay in review. I haven't looked into the details of PredecessorWALInfo creation and handling. Let me know if my comments are infeasible.

jowlyzhang · 2025-02-07T21:23:59Z

db/log_reader.cc

@@ -364,6 +366,7 @@ void Reader::MaybeVerifyPredecessorWALInfo(
      stop_replay_for_corruption_) {
    return;
  }
+  assert(corrupted_wal_number_);


nit: maybe add assertion for *corrupted_wal_number_ == kMaxSequenceNumber during construction.

jowlyzhang · 2025-02-07T21:31:55Z

db/db_impl/db_impl_open.cc

@@ -1400,7 +1403,7 @@ Status DBImpl::InitializeLogReader(
      immutable_db_options_.info_log, std::move(file_reader), reporter,
      true /*checksum*/, wal_number,
      immutable_db_options_.track_and_verify_wals, stop_replay_for_corruption,
-      min_wal_number, predecessor_wal_info));
+      min_wal_number, predecessor_wal_info, corrupted_wal_number));


It's a bit unusual to pass this state tracking variable into a log Reader's constructor. It is in some sense a output argument, but there is no clear syntax for it.

Can we instead track this in Reporter with an explicit API like GetCorruptedWalNumber?

Can we instead track this in Reporter

Agree - I decided to track the corrupted predecessor wal number in reporter instead. Fixed.

jowlyzhang · 2025-02-07T21:45:46Z

db/log_reader.cc

@@ -403,6 +409,7 @@ void Reader::MaybeVerifyPredecessorWALInfo(
          std::to_string(observed_predecessor_wal_info_.GetSizeBytes()) +
          " bytes.";
      ReportCorruption(fragment.size(), reason.c_str());


I'm thinking if we can use Reporter as the source of truth for the corrupted wal number, we can also update this ReportCorruption API to take an extra wal_number argument. It will either get the current wal number or the predecessor wal number passed depending on the type error.

Happy to chat more offline!

I'm thinking if we can use Reporter as the source of truth for the corrupted wal numbe

While it's a good direction to minimize the place we have "corrupted wal number", I found it difficult to use reporter as the source of truth for the corrupted_wal_number for the following reasons:

(1) Reporter, as the new name "DBOpenLogRecordReadReporter" suggests, it actually only tracks corruption per reading log record (e.g, physically corrupted log record, found corruption after reading the predecessor info record). But the corrupted_wal_number should be set even when there is log file level corruption e.g, fail to UpdatePredecessorWALInfo().

(2) Due to the messiness we still have with having multiple unconsolidated Status in ProcessLogFile(), I found it not easy to verify the log number at the time of ReportCorruption() will the be the same number as how we currently set corrupted_wal_number. This is to ensure no behavior change for all the corruption cases reported. That means it's better to accompany the change you suggested with status consolidation tech debt work.

Therefore, I decide to keep the "ugly" if-else statement in setting "corrupted_wal_number = ...." in HandleNonOkStatusOrOldLogRecord() for now.

we can also update this ReportCorruption API to take an extra wal_number argument

Done with some notes.

I do agree with using Reporter to keep track of the corrupted log number though I eventually decided to not using the Reporter::Corruption() to take another wal_number, but another API to deal with the special case where it's the prior WAL being corrupted.

This is because most of the corruption doesn't need to track log number inside of the reporter because the reporter will be automatically associated with the corrupted log file if any corruption happens. So I feel the wal_number parameter is mostly useless and ends up with something like below

.../rocksdb/db/repair.cc Status ConvertLogToTable(const std::string& wal_dir, uint64_t log) { struct LogReporter : public log::Reader::Reporter { Env* env; std::shared_ptr<Logger> info_log; uint64_t lognum; void Corruption(size_t bytes, const Status& s, uint64_t log_number /* another log number!!!!! */) override { // We print error messages for corruption, but continue repairing. ROCKS_LOG_ERROR(info_log, "Log #%" PRIu64 ": dropping %d bytes; %s", lognum, static_cast<int>(bytes), s.ToString().c_str()); } ..... }

facebook-github-bot · 2025-02-11T02:07:14Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-02-11T02:12:45Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-02-11T02:42:45Z

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

hx235 · 2025-02-11T02:43:47Z

db/db_impl/db_impl.h

@@ -162,15 +162,25 @@ class Directories {
  std::unique_ptr<FSDirectory> wal_dir_;
 };

-struct DBOpenLogReporter : public log::Reader::Reporter {
+struct DBOpenLogRecordReadReporter : public log::Reader::Reporter {


Rename to better indicate what level of corruption it reports. The level may change in the future once we figure out how to consolidate all the error status in ProcessLogFile() (see existing TODOs)

hx235 · 2025-02-11T02:44:26Z

db/db_impl/db_impl.h

-      Status log_read_status, bool* old_log_record,
-      bool* stop_replay_for_corruption, uint64_t* corrupted_wal_number,
-      bool* corrupted_wal_found);
+      Status status, const DBOpenLogRecordReadReporter& reporter,


Status log_read_status was a typo

hx235 · 2025-02-11T02:44:53Z

db/db_impl/db_impl_open.cc

@@ -1323,7 +1332,7 @@ Status DBImpl::ProcessLogFile(
  }

  ROCKS_LOG_INFO(immutable_db_options_.info_log,
-                 "Recovered to log #%" PRIu64 " seq #%" PRIu64, wal_number,
+                 "Recovered to log #%" PRIu64 " next seq #%" PRIu64, wal_number,


"seq" should be "next seq"

hx235 · 2025-02-11T02:46:17Z

db/log_reader.h

+  FragmentBufferedReader(std::shared_ptr<Logger> info_log,
+                         std::unique_ptr<SequentialFileReader>&& _file,
+                         Reporter* reporter, bool checksum, uint64_t log_num)
      : Reader(info_log, std::move(_file), reporter, checksum, log_num,


Only default values are used for these optional parameters in FragmentBufferedReader so removed them in the API

facebook-github-bot added the CLA Signed label Jan 31, 2025

hx235 requested a review from jowlyzhang February 3, 2025 19:59

hx235 requested a review from pdillinger February 4, 2025 04:32

jowlyzhang reviewed Feb 7, 2025

View reviewed changes

hx235 force-pushed the fix_actual_wal_num_corrupted branch from 26b7aad to 73aa9ee Compare February 11, 2025 02:07

hx235 changed the title ~~Fix corrupted wal number when predecessor wal corrupts~~ Fix corrupted wal number when predecessor wal corrupts + some cleanup Feb 11, 2025

hx235 changed the title ~~Fix corrupted wal number when predecessor wal corrupts + some cleanup~~ Fix corrupted wal number when predecessor wal corrupts + minor cleanup Feb 11, 2025

hx235 changed the title ~~Fix corrupted wal number when predecessor wal corrupts + minor cleanup~~ Fix corrupted wal number when predecessor wal corrupts + minor cleanup Feb 11, 2025

fix

da73398

hx235 force-pushed the fix_actual_wal_num_corrupted branch from 73aa9ee to da73398 Compare February 11, 2025 02:12

hx235 commented Feb 11, 2025

View reviewed changes

hx235 requested a review from jowlyzhang February 11, 2025 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix corrupted wal number when predecessor wal corrupts + minor cleanup #13359

Fix corrupted wal number when predecessor wal corrupts + minor cleanup #13359

hx235 commented Jan 31, 2025 •

edited

Loading

facebook-github-bot commented Feb 3, 2025

jowlyzhang left a comment

jowlyzhang Feb 7, 2025

hx235 Feb 11, 2025

jowlyzhang Feb 7, 2025

hx235 Feb 11, 2025

jowlyzhang Feb 7, 2025

hx235 Feb 11, 2025 •

edited

Loading

facebook-github-bot commented Feb 11, 2025

facebook-github-bot commented Feb 11, 2025

facebook-github-bot commented Feb 11, 2025

hx235 Feb 11, 2025

hx235 Feb 11, 2025

hx235 Feb 11, 2025

hx235 Feb 11, 2025

Fix corrupted wal number when predecessor wal corrupts + minor cleanup #13359

Are you sure you want to change the base?

Fix corrupted wal number when predecessor wal corrupts + minor cleanup #13359

Conversation

hx235 commented Jan 31, 2025 • edited Loading

facebook-github-bot commented Feb 3, 2025

jowlyzhang 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

Choose a reason for hiding this comment

hx235 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Feb 11, 2025

facebook-github-bot commented Feb 11, 2025

facebook-github-bot commented Feb 11, 2025

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

hx235 commented Jan 31, 2025 •

edited

Loading

hx235 Feb 11, 2025 •

edited

Loading