Fix corrupted wal number when predecessor wal corrupts + minor cleanup #13359
Conversation
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
db/log_reader.cc
Outdated
| @@ -403,6 +409,7 @@ void Reader::MaybeVerifyPredecessorWALInfo( | |||
| std::to_string(observed_predecessor_wal_info_.GetSizeBytes()) + | |||
| " bytes."; | |||
| ReportCorruption(fragment.size(), reason.c_str()); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Happy to chat more offline!
I'm thinking if we can use
Reporteras 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());
}
.....
}
There was a problem hiding this comment.
I'll leave this one open for people who are interested in learning about the context for the part we decided to change vs not change.
hx235
force-pushed
the
fix_actual_wal_num_corrupted
branch
from
26b7aad to
73aa9ee
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
hx235
changed the title
hx235
changed the title
hx235
changed the title
hx235
force-pushed
the
fix_actual_wal_num_corrupted
branch
from
73aa9ee to
da73398
Compare
|
@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. |
| @@ -162,15 +162,25 @@ class Directories { | |||
| std::unique_ptr<FSDirectory> wal_dir_; | |||
| }; | |||
|
|
|||
| struct DBOpenLogReporter : public log::Reader::Reporter { | |||
| struct DBOpenLogRecordReadReporter : public log::Reader::Reporter { | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
nit: alternatively, we can have a Reporter initialized with the wal_number_ it is opened for. If a Reporter::Corruption(, corrupted_wal_number=kMaxSequence) call is not providing a specific wal number, it will track its own wal_number_ as the corrupted_wal_number_. I think that can help eliminate the need of a dedicated PredecessorLogCorruption API. But it's up to you, not a big deal.
This may help us with removing the if else in HandleNonOkStatusOrOldLogRecord, but I agree with you that before we can consolidate the status handling, it's safer to keep it like this.
There was a problem hiding this comment.
I see let me think more and fix it!
There was a problem hiding this comment.
Fixed excluding the constructor part, which I found is better done with the status consolidation.
hx235
force-pushed
the
fix_actual_wal_num_corrupted
branch
from
da73398 to
f244289
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
Fix internal formatting issue |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
force-pushed
the
fix_actual_wal_num_corrupted
branch
from
f244289 to
e5dbf29
Compare
|
@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. |
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 namingTest: