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 #220] TiKV-BR support key range #221

Merged
merged 10 commits into from
Sep 14, 2022

Conversation

haojinming
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #220

Problem Description:

What is changed and how does it work?

Support key range in backup & restore.

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
  • Integration 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]>
Signed-off-by: haojinming <[email protected]>
Signed-off-by: haojinming <[email protected]>
Signed-off-by: haojinming <[email protected]>
Signed-off-by: haojinming <[email protected]>
Signed-off-by: haojinming <[email protected]>
@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #221 (3ad25d0) into main (088ea7b) will decrease coverage by 0.1071%.
The diff coverage is 22.2222%.

❗ Current head 3ad25d0 differs from pull request most recent head 99f9f02. Consider uploading reports for the commit 99f9f02 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##               main       #221        +/-   ##
================================================
- Coverage   54.4832%   54.3760%   -0.1072%     
================================================
  Files           236        236                
  Lines         20298      20178       -120     
================================================
- Hits          11059      10972        -87     
+ Misses         8361       8329        -32     
+ Partials        878        877         -1     
Flag Coverage Δ
br 38.9352% <22.2222%> (-0.6227%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
br/pkg/restore/client.go 0.0000% <0.0000%> (ø)
br/pkg/restore/util.go 68.1818% <ø> (ø)
br/pkg/storage/s3.go 55.8312% <ø> (+0.2757%) ⬆️
br/pkg/task/backup_raw.go 0.0000% <ø> (ø)
br/pkg/task/common.go 18.4782% <ø> (+0.3931%) ⬆️
br/pkg/task/restore_raw.go 0.0000% <ø> (ø)
br/pkg/utils/backoff.go 54.9450% <0.0000%> (ø)
br/pkg/version/build/info.go 100.0000% <ø> (ø)
br/pkg/version/version.go 82.9268% <ø> (-2.8840%) ⬇️
br/pkg/conn/conn.go 27.9661% <100.0000%> (+0.2350%) ⬆️
... and 2 more

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

@zeminzhou zeminzhou left a comment

Choose a reason for hiding this comment

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

LGTM~


require.True(t, g.OwnsStorage())

ctx := context.Background()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is verified in this block ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to cover the code to verify no crash occurs.

if err != nil {
return err
}
totalCnt += batchCnt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be atomic addition.

{StartKey: []byte{}, EndKey: []byte{}},
{StartKey: q1Key, EndKey: q3Key},
}
for _, keyRange := range backupRanges {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: it seems better to treat backupRanges as test cases, and move the loop to main.

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

log.Panic("ClearStorage fail", zap.Error(err))
}
safeInterval := int64(120) // 2m
backupOutput, err := tester.Backup(ctx, kvrpcpb.APIVersion_V2, safeInterval, keyRange.StartKey, keyRange.EndKey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to also verify the backup result by restore + checksum.

Copy link
Contributor Author

@haojinming haojinming Sep 13, 2022

Choose a reason for hiding this comment

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

There is a bit problem here, we only deploy one cluster in integration test. May need deploy apiv1 & apiv2 clusters to verify the conversion result.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. We may consider to deploy two clusters for this test case later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok,I will improve it later.

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 1640834 into tikv:main Sep 14, 2022
@haojinming haojinming deleted the br_support_more_args branch September 14, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TiKV-BR Support keyrange in backup&restore
3 participants