Enable file checksum for clone startup#1489
Conversation
| temp.c_str()); | ||
| } | ||
|
|
||
| myrocks::rocksdb_file_checksums = |
There was a problem hiding this comment.
This does not appear to be the best place for this. Why not in fixup_on_startup in the same file?
There was a problem hiding this comment.
Moved it to the fixup_on_startup before renaming temp datadir to datadir.
There was a problem hiding this comment.
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
force-pushed
the
fb-mysql-8.0.32-use-checksum-for-clone
branch
from
dcc1441 to
b6e7d94
Compare
|
@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hermanlee
force-pushed
the
fb-mysql-8.0.32
branch
from
82f97d7 to
7c087d2
Compare
sunshine-Chun
force-pushed
the
fb-mysql-8.0.32-use-checksum-for-clone
branch
from
b6e7d94 to
ca49d1b
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
| // 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)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You are right, the current code is OK
| static const char *file_checksums_names[] = { | ||
| "CHECKSUMS_OFF", "CHECKSUMS_WRITE_ONLY", "CHECKSUMS_WRITE_AND_VERIFY", | ||
| NullS}; | ||
| "CHECKSUMS_WRITE_AND_VERIFY_ON_CLONE", NullS}; |
There was a problem hiding this comment.
Are there tests that need re-recording? main.mysqld--help-notwin, maybe others?
There was a problem hiding this comment.
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
force-pushed
the
fb-mysql-8.0.32-use-checksum-for-clone
branch
from
ca49d1b to
b69bea3
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
|
@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
sunshine-Chun
force-pushed
the
fb-mysql-8.0.32-use-checksum-for-clone
branch
from
b69bea3 to
6041c44
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
|
@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -0,0 +1 @@ | |||
| --rocksdb-file-checksums=CHECKSUMS_WRITE_AND_VERIFY_ON_CLONE | |||
There was a problem hiding this comment.
Can this be moved to the testcase restart_parameter settings?
There was a problem hiding this comment.
Moved to restart parameter.
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
sunshine-Chun
force-pushed
the
fb-mysql-8.0.32-use-checksum-for-clone
branch
from
6041c44 to
f2418fa
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
|
@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request has been merged in 6389d8d. |
3 participants
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: