Skip to content

Enable file checksum for clone startup #1489

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
Closed
sunshine-Chun wants to merge 1 commit into from
Closed

Enable file checksum for clone startup #1489

sunshine-Chun wants to merge 1 commit into from

Conversation

@sunshine-Chun
Copy link
Contributor

@sunshine-Chun sunshine-Chun commented Sep 4, 2024 •
edited
Loading

Summary: This change is to enable file checksum check only for recipients of clone. For regular startup, we don't do the sst file checksum check.

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
laurynas-biveinis
laurynas-biveinis reviewed Sep 4, 2024
View reviewed changes
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to test this by adding a clone MTR test that corrupts a few bytes before the cloned instance startup?
storage/rocksdb/clone/common.cc Outdated
temp.c_str());
}

myrocks::rocksdb_file_checksums =
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis Sep 4, 2024

Choose a reason for hiding this comment

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

This does not appear to be the best place for this. Why not in fixup_on_startup in the same file?
Copy link
Contributor Author

@sunshine-Chun sunshine-Chun Sep 4, 2024

Choose a reason for hiding this comment

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

Moved it to the fixup_on_startup before renaming temp datadir to datadir.
Copy link
Contributor Author

@sunshine-Chun sunshine-Chun Sep 4, 2024

Choose a reason for hiding this comment

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

Tested in test server. In the restart during clone, it triggers the checksum verification.

2024-09-04T14:05:04.364182-07:00 0 [Note] [MY-011071] [Server] Plugin rocksdb reported: 'Verifying file checksums...'
2024-09-04T14:05:04.364244-07:00 0 [Note] [MY-011071] [Server] Plugin rocksdb reported: '...done'

Restart the recipient instance after clone finished, no checksum verification is triggered cause there is no related log reported by plugin rocksdb.
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.32-use-checksum-for-clone branch from dcc1441 to b6e7d94 Compare September 4, 2024 21:33
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 4, 2024

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.32-use-checksum-for-clone branch from b6e7d94 to ca49d1b Compare September 4, 2024 22:09
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 4, 2024

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.
laurynas-biveinis
laurynas-biveinis reviewed Sep 5, 2024
View reviewed changes
storage/rocksdb/clone/common.cc
// after a clone.
if (myrocks::rocksdb_file_checksums ==
myrocks::file_checksums_type::CHECKSUMS_WRITE_AND_VERIFY_ON_CLONE &&
temp_dir_exists_abort_if_not_dir(in_place_temp_datadir)) {
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis Sep 5, 2024

Choose a reason for hiding this comment

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

Thank you, this is a better location IMHO.I I'd also remove temp_dir_exists_abort_if_not_dir check for clarity, but up to you
Copy link
Contributor Author

@sunshine-Chun sunshine-Chun Sep 5, 2024

Choose a reason for hiding this comment

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

If temp_dir_exists_abort_if_not_dir is removed, will the restart happens on every mysqld restart, not just the restart right after clone? The intended behavior is the checksum verification only happens for clone.
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis Sep 6, 2024

Choose a reason for hiding this comment

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

You are right, the current code is OK
storage/rocksdb/ha_rocksdb.cc
static const char *file_checksums_names[] = {
"CHECKSUMS_OFF", "CHECKSUMS_WRITE_ONLY", "CHECKSUMS_WRITE_AND_VERIFY",
NullS};
"CHECKSUMS_WRITE_AND_VERIFY_ON_CLONE", NullS};
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis Sep 5, 2024

Choose a reason for hiding this comment

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

Are there tests that need re-recording? main.mysqld--help-notwin, maybe others?
Copy link
Contributor Author

@sunshine-Chun sunshine-Chun Sep 6, 2024

Choose a reason for hiding this comment

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

Doesn't seem so. The related MTR test passes. But I add one more MTR test for this change. It's included in the latest iteration.
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.32-use-checksum-for-clone branch from ca49d1b to b69bea3 Compare September 5, 2024 16:52
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 5, 2024

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 5, 2024

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.32-use-checksum-for-clone branch from b69bea3 to 6041c44 Compare September 5, 2024 17:51
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 5, 2024

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 5, 2024

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
laurynas-biveinis
laurynas-biveinis reviewed Sep 6, 2024
View reviewed changes
mysql-test/suite/rocksdb_clone/t/checksum_verification_after_clone-master.opt Outdated
@@ -0,0 +1 @@
--rocksdb-file-checksums=CHECKSUMS_WRITE_AND_VERIFY_ON_CLONE
Copy link
Contributor

@laurynas-biveinis laurynas-biveinis Sep 6, 2024

Choose a reason for hiding this comment

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

Can this be moved to the testcase restart_parameter settings?
Copy link
Contributor Author

@sunshine-Chun sunshine-Chun Sep 6, 2024

Choose a reason for hiding this comment

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

Moved to restart parameter.
@sunshine-Chun
Enable rocksdb file checksum after clone
f2418fa 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@sunshine-Chun sunshine-Chun force-pushed the fb-mysql-8.0.32-use-checksum-for-clone branch from 6041c44 to f2418fa Compare September 6, 2024 17:57
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 6, 2024

@sunshine-Chun has updated the pull request. You must reimport the pull request before landing.
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 6, 2024

@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@facebook-github-bot
Copy link

facebook-github-bot commented Sep 6, 2024

This pull request has been merged in 6389d8d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Merged

3 participants

@sunshine-Chun @facebook-github-bot @laurynas-biveinis