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 #231] TiKV-BR support v5.x.x #282

Merged
merged 25 commits into from
Nov 4, 2022
Merged

Conversation

haojinming
Copy link
Contributor

Signed-off-by: haojinming jinming.hao@pingcap.com

What problem does this PR solve?

Issue Number: close #231

Problem Description: =

What is changed and how does it work?

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 <jinming.hao@pingcap.com>
@haojinming haojinming marked this pull request as draft November 1, 2022 07:39
@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Merging #282 (45b6f8d) into main (c4c08d6) will decrease coverage by 0.0878%.
The diff coverage is 12.3188%.

❗ Current head 45b6f8d differs from pull request most recent head 81bb104. Consider uploading reports for the commit 81bb104 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##               main       #282        +/-   ##
================================================
- Coverage   54.4448%   54.3570%   -0.0879%     
================================================
  Files           238        239         +1     
  Lines         20237      20312        +75     
================================================
+ Hits          11018      11041        +23     
- Misses         8341       8393        +52     
  Partials        878        878                
Flag Coverage Δ *Carryforward flag
br 39.4736% <20.4819%> (-0.0624%) ⬇️
cdc 61.2383% <0.0000%> (-0.0693%) ⬇️ Carriedforward from 80f7a2d

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
br/cmd/br/debug.go 26.9841% <0.0000%> (-1.0379%) ⬇️
br/pkg/backup/push.go 32.5581% <0.0000%> (-3.5764%) ⬇️
br/pkg/task/backup_raw.go 0.0000% <0.0000%> (ø)
br/pkg/task/restore_raw.go 0.0000% <0.0000%> (ø)
br/pkg/version/version.go 82.9268% <ø> (ø)
cdc/cdc/capture/capture.go 0.0000% <0.0000%> (ø)
cdc/cdc/capture/http_handler.go 0.0000% <0.0000%> (ø)
cdc/cdc/server.go 36.0189% <0.0000%> (ø)
br/pkg/feature/feature_gate.go 100.0000% <100.0000%> (ø)
br/pkg/pdutil/pd.go 40.2777% <100.0000%> (+0.3336%) ⬆️
... and 13 more

Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
@haojinming haojinming marked this pull request as ready for review November 2, 2022 09:18
@haojinming haojinming requested review from zeminzhou and pingyu and removed request for zeminzhou November 2, 2022 09:18
Store: store,
}
})
failpoint.Inject("tikv-region-error", func(val failpoint.Value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have we verified that the failpoint is effective ? This failpoint should cause an incomplete range and enter procedure fineGrainedBackup. I remember that there was a message "start fine grained backup" in log printed here, but I can't find it in CI logs.

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 seems be an issue. Now it works, see following:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change will change the original behavior, as all regions will fail, other than only one of them. In real scenarios, there should be only a small number of regions will fail.

I think we can change FAILPOINTS of rawkv test from return("region error") to 1*return("region error") to control how many times the failpoint takes effective.
Refer to here (this change add a "one region fail" case in addition to "all regions").

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, thanks.

haojinming and others added 7 commits November 4, 2022 10:45
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
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~

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.

br/Makefile Outdated
@@ -31,6 +31,7 @@ TEST_PARALLEL ?= 8
PD_ADDR ?= 127.0.0.1:2379
BR_LOCAL_STORE ?= /tmp/backup_restore_test
API_VERSION ?= 1
CLUSTER_VERSION?= nightly
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: add a space before ?= and align other lines.

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 <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
Signed-off-by: haojinming <jinming.hao@pingcap.com>
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 enabled auto-merge (squash) November 4, 2022 10:46
@haojinming
Copy link
Contributor Author

/verify

@pingyu pingyu disabled auto-merge November 4, 2022 14:33
@pingyu
Copy link
Collaborator

pingyu commented Nov 4, 2022

Force merge. Fix cdc integrated tests in another PR.
See the log:
https://ci.pingcap.net/blue/organizations/jenkins/common-check/detail/common-check/112025/pipeline/

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.

Support backup TiKV 5.X version raw data in TiKV-BR
3 participants