Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Begin forward compatibility for WAL entry #13225

Closed
wants to merge 1 commit into from
Closed

Begin forward compatibility for WAL entry #13225

wants to merge 1 commit into from

Conversation

hx235
Copy link
Contributor

@hx235 hx235 commented Dec 19, 2024

Context/Summary:

This PR made WAL reader safely ignore type that is greater than some hard-coded value so old code can ignore new WAL
entry developed by new code.

Test:
Manually run crash test alternating on old (this PR) and new code (this PR + a new WAL entry kPredecessorWALInfo hx235@37e8b1a)

Running db_stress with pid=3201845: /data/users/huixiao/rocksdb/db_stress_new ....

KILLED 3201845
 
Running db_stress with pid=3203911: /data/users/huixiao/rocksdb_2/rocksdb/db_stress_old ...

KILLED 3203911

Running db_stress with pid=3218107: /data/users/huixiao/rocksdb/db_stress_new ....

db_crashtest.py

diff --git a/tools/db_crashtest.py b/tools/db_crashtest.py
index 59912dbe9..b1d505367 100644
--- a/tools/db_crashtest.py
+++ b/tools/db_crashtest.py
@@ -1074,6 +1072,10 @@ def gen_cmd_params(args):
 
 def gen_cmd(params, unknown_params):
     finalzied_params = finalize_and_sanitize(params)
+    if random.randint(0, 1) == 0:
+        stress_cmd = "/data/users/huixiao/rocksdb/db_stress_new"
+    else:
+        stress_cmd = "/data/users/huixiao/rocksdb_2/rocksdb/db_stress_old"
     cmd = (
         [stress_cmd]
         + [

New code
hx235@37e8b1a

--- a/db_stress_tool/db_stress_test_base.cc
+++ b/db_stress_tool/db_stress_test_base.cc
@@ -4117,7 +4117,7 @@ void InitializeOptionsFromFlags(
   options.level_compaction_dynamic_level_bytes =
       FLAGS_level_compaction_dynamic_level_bytes;
   options.track_and_verify_wals_in_manifest = true;
-  options.track_and_verify_wals = FLAGS_track_and_verify_wals;
+  options.track_and_verify_wals = true;
   options.verify_sst_unique_id_in_manifest =
       FLAGS_verify_sst_unique_id_in_manifest;

hx235
hx235 commented Dec 19, 2024
View reviewed changes
db/log_format.h Outdated
};
constexpr int kMaxRecordType = kRecyclableUserDefinedTimestampSizeType;
const int kRecordTypeSafeIgnoreMask = 1 << 5;
constexpr int kMaxRecordType = kRecordTypeSafeIgnoreMask + 5;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will soon to be overwritten to be the last entry type in WAL that is greater than kRecordTypeSafeIgnoreMask in my upcoming PR.

@facebook-github-bot
Copy link
Contributor

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

@hx235 hx235 mentioned this pull request Dec 19, 2024
Closed
pdillinger
pdillinger requested changes Dec 19, 2024
View reviewed changes
db/log_reader.cc Outdated
ReportCorruption(
(fragment.size() + (in_fragmented_record ? scratch->size() : 0)),
reason.c_str());
if (record_type <= kRecordTypeSafeIgnoreMask) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a "mask", we should check it as a bit mask rather than a minimum value. Unless we have a sense of which is going to be most common, I suggest we split the remaining value space roughly in half for ignorable vs. rejected when unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

db/log_test.cc
Write("foo");
// Type is stored in header[6]
IncrementByte(6, 100);
FixChecksum(0, 3, false);
ASSERT_EQ("EOF", Read());
ASSERT_EQ(3U, DroppedBytes());
ASSERT_EQ("OK", MatchError("unknown record type"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep be testing both the accepted and ignored unknown record type and rejected unknown record type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@hx235 hx235 force-pushed the compatible_wal_entry branch from dc07c04 to bc3ae2c Compare December 19, 2024 23:17
@facebook-github-bot
Copy link
Contributor

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

pdillinger
pdillinger approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

db/log_test.cc Outdated
TEST_P(LogTest, IgnorableRecordType) {
Write("foo");
// Type is stored in header[6]
SetByte(6, static_cast<char>(kRecordTypeSafeIgnoreMask + 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use these in order, I recommend something like + 100 instead of + 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@facebook-github-bot
Copy link
Contributor

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

@hx235 hx235 force-pushed the compatible_wal_entry branch from bc3ae2c to 02a86b9 Compare December 20, 2024 01:41
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@hx235 merged this pull request in cf768a2.

hx235 added a commit to hx235/rocksdb that referenced this pull request Dec 20, 2024
@hx235
debug + Revert "Begin forward compatibility for WAL entry (facebook#1…
e783fc1 
…3225)"

This reverts commit cf768a2.
hx235 added a commit to hx235/rocksdb that referenced this pull request Dec 20, 2024
@hx235
debug + Revert "Begin forward compatibility for WAL entry (facebook#1…
28e01d5 
…3225)"

This reverts commit cf768a2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdillinger pdillinger pdillinger approved these changes

@jowlyzhang jowlyzhang Awaiting requested review from jowlyzhang

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hx235 @facebook-github-bot @pdillinger