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.

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

[close #108] Br debug checksum cmd #114

Merged
merged 4 commits into from
Jun 9, 2022

Conversation

haojinming
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #108

Problem Description:
Checksum maybe mismatch if data expired during backup/restore.

What is changed and how does it work?

  1. Add a warning that "Warning: TiKV cluster is TTL enabled, checksum may be mismatch if some data expired during backup/restore."
  2. Implement checksum debug command.
  3. Add api version check to reject the invalid backup/restore operation.

Code changes

  • Has exported function/method change

Check List for Tests

This PR has been tested by at least one of the following methods:

  • Unit test
  • Manual test (add detailed scripts or steps below)

Side effects

  • No side effects

Related changes

  • No related changes

Signed-off-by: haojinming <[email protected]>

add warning

Signed-off-by: haojinming <[email protected]>

revert changes

Signed-off-by: haojinming <[email protected]>
@haojinming haojinming requested a review from pingyu June 9, 2022 02:12
Copy link
Collaborator

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

Rest LGTM~

@@ -64,12 +67,17 @@ func NewRestoreClient(
keepaliveConf keepalive.ClientParameters,
isRawKv bool,
) (*Client, error) {
apiVerion, err := backup.GetTiKVApiVersion(context.Background(), pdClient, tlsConf)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little strange to use a method from backup in restore procedure. It would be better to move GetTiKVApiVersion as a common method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to common conn package, thanks for the good suggestion.

@@ -58,10 +58,10 @@ func DefineRawBackupFlags(command *cobra.Command) {
}

// CalcChecksumFromBackupMeta read the backup meta and return Checksum
func CalcChecksumFromBackupMeta(ctx context.Context, curAPIVersion kvrpcpb.APIVersion, cfg *Config) (checksum.Checksum, []*utils.KeyRange, error) {
func CalcChecksumFromBackupMeta(ctx context.Context, curAPIVersion kvrpcpb.APIVersion, cfg *Config) (*backuppb.BackupMeta, checksum.Checksum, []*utils.KeyRange, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: maybe better to get backupMeta outside then pass in here. As what "CalcChecksumFromBackupMeta" says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: haojinming <[email protected]>
Copy link
Collaborator

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

LGTM~

@pingyu pingyu merged commit b8eef52 into tikv:main Jun 9, 2022
@haojinming haojinming deleted the br_debug_checksum_cmd branch June 14, 2022 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TiKV-BR] checksum may mismatch when enabling ttl
2 participants