br: revert new introduced `BatchScanRegions` to keep compatibility #54365

you06 · 2024-07-01T09:07:44Z

What problem does this PR solve?

Issue Number: close #54313 #54341

Problem Summary:

What changed and how does it work?

We have deprecated the ScanRegion and introduce BatchScanRegions as a replacement in v8.2. However, to keep compatibility(v8.2 br/lightning should work with previous TiDB clusters), the tools should keep using the old supported ScanRegion.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

The integration test case which import into 8.1 tidb with this PR's lightning is passed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

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

tiprow · 2024-07-01T09:08:04Z

Hi @you06. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2024-07-01T09:35:32Z

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.5109%. Comparing base (c983546) to head (fe50606).
Report is 34 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54365         +/-   ##
=================================================
- Coverage   72.8081%   56.5109%   -16.2972%     
=================================================
  Files          1527       1679        +152     
  Lines        435913     622548     +186635     
=================================================
+ Hits         317380     351808      +34428     
- Misses        98884     246385     +147501     
- Partials      19649      24355       +4706

Flag	Coverage Δ
integration	`38.3881% <40.0000%> (?)`
unit	`71.8139% <40.0000%> (+0.0142%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`58.6880% <33.3333%> (+12.5732%)`	⬆️

you06 · 2024-07-01T10:20:38Z

/retest

tiprow · 2024-07-01T10:21:00Z

@you06: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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

D3Hunter · 2024-07-03T06:19:58Z

br/tests/br_key_locked/codec.go

@@ -63,7 +63,8 @@ func (c *codecPDClient) ScanRegions(
 		endKey = codec.EncodeBytes(nil, endKey)
 	}

-	regions, err := c.Client.BatchScanRegions(ctx, []pd.KeyRange{{StartKey: startKey, EndKey: endKey}}, limit, opts...)
+	//nolint:staticcheck
+	regions, err := c.Client.ScanRegions(ctx, startKey, endKey, limit, opts...)


those can be removed if we have do it in pd client-go?

Sorry I don't get your mean, I just revert it to the old version.

i mean if we have make interface compatible in client-go side, we can still use BatchScanRegions here

It's a little weird that pd's client handle unimplement error, and the compatibility is made out of pd client. So I switch back to old interface here.

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

ti-chi-bot · 2024-07-03T11:27:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, YuJuncen

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [YuJuncen,cfzjywxk]
~~br/OWNERS~~ [YuJuncen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-07-03T11:27:34Z

[LGTM Timeline notifier]

Timeline:

2024-07-03 11:12:51.595983336 +0000 UTC m=+1409298.081472168: ☑️ agreed by cfzjywxk.
2024-07-03 11:27:33.715596864 +0000 UTC m=+1410180.201085696: ☑️ agreed by YuJuncen.

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

you06 · 2024-07-03T12:28:17Z

/retest

tiprow · 2024-07-03T12:28:38Z

@you06: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

cfzjywxk · 2024-07-03T14:38:40Z

/ok-to-test

ti-chi-bot · 2024-07-03T14:52:45Z

In response to a cherrypick label: new pull request created to branch release-8.2: #54425.

…54365) (#54425) close #54313

revert new introduced to keep compability

9c65455

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

ti-chi-bot bot added do-not-merge/needs-triage-completed do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2024

you06 changed the title ~~br: revert new introduced BatchScanRegions to keep compability~~ br: revert new introduced BatchScanRegions to keep compatibility Jul 1, 2024

update client-go

a8fb928

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

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 3, 2024

D3Hunter reviewed Jul 3, 2024

View reviewed changes

update client-go

a5e3060

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

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jul 3, 2024

cfzjywxk approved these changes Jul 3, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 3, 2024

YuJuncen approved these changes Jul 3, 2024

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 3, 2024

fix mock pd

fe50606

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

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 3, 2024

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Jul 3, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Jul 3, 2024

ti-chi-bot bot merged commit c71eece into pingcap:master Jul 3, 2024
24 of 33 checks passed

you06 added the needs-cherry-pick-release-8.2 label Jul 3, 2024

ti-chi-bot mentioned this pull request Jul 3, 2024

br: revert new introduced BatchScanRegions to keep compatibility (#54365) #54425

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jul 4, 2024

br: revert new introduced BatchScanRegions to keep compatibility (#…

dc816da

…54365) (#54425) close #54313

you06 mentioned this pull request Jul 4, 2024

tpcc report error “GROUP BY ol_d_id failed Error 1105: PD returned no region, range num: 2, limit: 128” #54375

Closed

lance6716 mentioned this pull request Jul 4, 2024

tidb lightning encountered error: [Lightning:PreCheck:ErrCheckTableEmpty]check table empty error: check table contains data failed: check table empty failed: Error 1105 (HY000): PD returned no region, range num: 1, limit: 128 #54277

Closed

AkiraXie mentioned this pull request Jul 5, 2024

lightning: v8.2 lightning failed to import data to v8.1 version tidb cluster #54341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: revert new introduced `BatchScanRegions` to keep compatibility #54365

br: revert new introduced `BatchScanRegions` to keep compatibility #54365

you06 commented Jul 1, 2024 •

edited

Loading

tiprow bot commented Jul 1, 2024

codecov bot commented Jul 1, 2024 •

edited

Loading

you06 commented Jul 1, 2024

tiprow bot commented Jul 1, 2024

D3Hunter Jul 3, 2024

you06 Jul 3, 2024

D3Hunter Jul 3, 2024

you06 Jul 4, 2024

ti-chi-bot bot commented Jul 3, 2024

ti-chi-bot bot commented Jul 3, 2024

you06 commented Jul 3, 2024

tiprow bot commented Jul 3, 2024

cfzjywxk commented Jul 3, 2024

ti-chi-bot commented Jul 3, 2024

br: revert new introduced BatchScanRegions to keep compatibility #54365

br: revert new introduced BatchScanRegions to keep compatibility #54365

Conversation

you06 commented Jul 1, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 1, 2024

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

you06 commented Jul 1, 2024

tiprow bot commented Jul 1, 2024

D3Hunter Jul 3, 2024

Choose a reason for hiding this comment

you06 Jul 3, 2024

Choose a reason for hiding this comment

D3Hunter Jul 3, 2024

Choose a reason for hiding this comment

you06 Jul 4, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 3, 2024

ti-chi-bot bot commented Jul 3, 2024

[LGTM Timeline notifier]

you06 commented Jul 3, 2024

tiprow bot commented Jul 3, 2024

cfzjywxk commented Jul 3, 2024

ti-chi-bot commented Jul 3, 2024

br: revert new introduced `BatchScanRegions` to keep compatibility #54365

br: revert new introduced `BatchScanRegions` to keep compatibility #54365

you06 commented Jul 1, 2024 •

edited

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading