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

backup, restore: Write correct api-version in rawkv backup/restore #31122

Merged
merged 76 commits into from
Feb 8, 2022
Merged

backup, restore: Write correct api-version in rawkv backup/restore #31122

merged 76 commits into from
Feb 8, 2022

Conversation

peng1999
Copy link
Contributor

@peng1999 peng1999 commented Dec 29, 2021

Signed-off-by: Peng Guanwen pg999w@outlook.com

What problem does this PR solve?

Issue Number: close #31121

Problem Summary:

BR backup should know the current API version of TiKV and write it in backupmeta.

What is changed and how it works?

Only RawKV backup/restore is changed because TiDB data is compatible across API V1 and V2.

Check List

Tests

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

Release note

None

ref #31121

Signed-off-by: Peng Guanwen <pg999w@outlook.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 29, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • andylokandy
  • kennytm

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 29, 2021
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
@kennytm
Copy link
Contributor

kennytm commented Dec 30, 2021

[2021-12-29T08:09:55.616Z] /go/pkg/mod/github.com/tikv/client-go/v2@v2.0.0-rc.0.20211223062159-300275dee63e/internal/mockstore/mocktikv/pd.go:70:9: cannot use &pdClient{...} (type *pdClient) as type pd.Client in return argument:

[2021-12-29T08:09:55.616Z] 	*pdClient does not implement pd.Client (missing LoadGlobalConfig method)

[2021-12-29T08:09:55.616Z] # github.com/tikv/client-go/v2/internal/mockstore/mocktikv

[2021-12-29T08:09:55.616Z] /go/pkg/mod/github.com/tikv/client-go/v2@v2.0.0-rc.0.20211223062159-300275dee63e/internal/mockstore/mocktikv/pd.go:70:9: cannot use &pdClient{...} (type *pdClient) as type pd.Client in return argument:

[2021-12-29T08:09:55.616Z] 	*pdClient does not implement pd.Client (missing LoadGlobalConfig method)

@sre-bot
Copy link
Contributor

sre-bot commented Dec 31, 2021

Copy link
Contributor

@kennytm kennytm 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

go.mod Outdated
@@ -103,3 +103,6 @@ replace github.com/pingcap/tidb/parser => ./parser

// fix potential security issue(CVE-2020-26160) introduced by indirect dependency.
replace github.com/dgrijalva/jwt-go => github.com/form3tech-oss/jwt-go v3.2.6-0.20210809144907-32ab6a8243d7+incompatible

// See https://github.com/pingcap/tidb/pull/31010
replace github.com/tikv/client-go/v2 => github.com/lemonhx/client-go/v2 v2.0.0-rc.0.20211224062518-60bb3ea439ab
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean this PR is blocked by #31010?

Copy link
Contributor Author

@peng1999 peng1999 Dec 31, 2021

Choose a reason for hiding this comment

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

This is blocked by tikv/client-go#405. The root cause of the compilation error is an API breakage introduced in pingcap/kvproto#828. The fix here is copied from #31010.

@peng1999 peng1999 changed the title backup: Write corresponding api-version to backupmeta in rawkv backup backup, restore: Write corresponding api-version to backupmeta in rawkv backup/restore Dec 31, 2021
@peng1999 peng1999 changed the title backup, restore: Write corresponding api-version to backupmeta in rawkv backup/restore backup, restore: Write correct api-version in rawkv backup/restore Dec 31, 2021
Signed-off-by: Peng Guanwen <pg999w@outlook.com>
@kennytm
Copy link
Contributor

kennytm commented Dec 31, 2021

/hold on tikv/client-go#405

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 31, 2021
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 31, 2021
@peng1999
Copy link
Contributor Author

If #31219 (likely to get merged early than tikv/client-go#405) gets merged, this one will also be unblocked.

@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 4, 2022
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 5, 2022
@Mini256
Copy link
Member

Mini256 commented Jan 24, 2022

/hold

In order to avoid the waste of the CI resource caused by the all test rerun, which is triggered by the merge base operation, I hold this PR, and it can be canceled through the /unhold command after the test fails problem to be resolved.

cc: @kennytm @hawkingrei

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2022
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2022
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed status/can-merge Indicates a PR has been approved by a committer. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 8, 2022
@andylokandy
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 3f07ded

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 8, 2022
@Mini256
Copy link
Member

Mini256 commented Feb 8, 2022

/unhold

It seems ready to merge.

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2022
@ti-chi-bot ti-chi-bot merged commit 74261e1 into pingcap:master Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check API V2 in BR
7 participants