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.

Sign up for GitHub

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

Jump to bottom

store/copr: batch build coprocessor tasks #54153

Merged
merged 3 commits into from
Jun 24, 2024
Merged

store/copr: batch build coprocessor tasks #54153

merged 3 commits into from
Jun 24, 2024

Conversation

you06
Copy link
Contributor

@you06 you06 commented Jun 21, 2024

What problem does this PR solve?

Issue Number: close #53850

Problem Summary:

What changed and how does it work?

Build coprocessor tasks from the new interface BatchLocateKeyRanges.

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 test result:

  1. split table into 1000 regions by SPLIT TABLE sbtest1 BETWEEN (0) AND (1000000) REGIONS 1000;
  2. wait until region cache expired.
  3. run a simple query explain analyze select * from sbtest1 limit 1;
  • Without this interface: ...build_task_duration: 81.5ms..., statmenet execution takes 0.11 sec
  • With this interface: ...build_task_duration: 2.7ms..., statement execution takes 0.02 sec

Side effects

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

Optimize the performance of building coprocessor tasks by batch locating regions.

@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2024
@tiprow tiprow
Copy link

tiprow bot commented Jun 21, 2024

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.

@you06
Copy link
Contributor Author

you06 commented Jun 21, 2024

/ok-to-test

@ti-chi-bot ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Jun 21, 2024
@you06
Copy link
Contributor Author

you06 commented Jun 21, 2024

/retest

@codecov Codecov
Copy link

codecov bot commented Jun 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.

Project coverage is 55.9413%. Comparing base (ea565fc) to head (cd85208).
Report is 10 commits behind head on master.

Additional details and impacted files 
@@                Coverage Diff                @@
##             master     #54153         +/-   ##
=================================================
- Coverage   72.8336%   55.9413%   -16.8924%     
=================================================
  Files          1520       1666        +146     
  Lines        434902     618051     +183149     
=================================================
+ Hits         316755     345746      +28991     
- Misses        98549     248200     +149651     
- Partials      19598      24105       +4507
Flag Coverage Δ
integration 22.9445% <23.1707%> (?)

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

Components Coverage Δ
dumpling 52.9656% <ø> (ø)
parser ∅ <ø> (∅)
br 62.9111% <85.7142%> (+16.7554%) ⬆️

@you06
fix bug
006cbc9 
Signed-off-by: you06 <[email protected]>

fix bug

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

remove with leader option

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

update go.mod

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

update client-go

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

bazel

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

comment public func

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

update client-go

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

update bazel

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

deprecate ScanRegions

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

bazel

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

sort imports

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

impl BatchScanRegions for MockPDClientForSplit

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

fix injected error

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

impl BatchScanRegions for FakePDClient

Signed-off-by: you06 <[email protected]>
@you06
Copy link
Contributor Author

you06 commented Jun 24, 2024

/retest

2 similar comments
@you06
Copy link
Contributor Author

you06 commented Jun 24, 2024

/retest

@you06
Copy link
Contributor Author

you06 commented Jun 24, 2024

/retest

@you06
update client-go
7544902 
Signed-off-by: you06 <[email protected]>
@you06
update client-go
cd85208 
Signed-off-by: you06 <[email protected]>
@you06
Copy link
Contributor Author

you06 commented Jun 24, 2024

/retest

1 similar comment
@you06
Copy link
Contributor Author

you06 commented Jun 24, 2024

/retest

you06
you06 commented Jun 24, 2024
View reviewed changes
pkg/store/copr/batch_coprocessor.go
@@ -1483,7 +1483,7 @@ func buildBatchCopTasksConsistentHashForPD(bo *backoff.Backoffer,
splitKeyStart := time.Now()
for i, ranges := range rangesForEachPhysicalTable {
rangesLen += ranges.Len()
locations, err := cache.SplitKeyRangesByLocationsWithoutBuckets(bo, ranges, UnspecifiedLimit)
locations, err := cache.SplitKeyRangesByLocations(bo, ranges, UnspecifiedLimit, false, false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@windtalker PTAL this file, I unify SplitKeyRangesByLocationsWithBuckets and SplitKeyRangesByLocationsWithoutBuckets. For batch_coprocessor, region without leader peer will also be returned(though this case is impossible in our expection).

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

YuJuncen
YuJuncen reviewed Jun 24, 2024
View reviewed changes
Copy link
Contributor

@YuJuncen YuJuncen left a comment

Choose a reason for hiding this comment

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

BR part LGTM.

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 24, 2024
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 24, 2024
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 24, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-06-24 09:43:18.660316054 +0000 UTC m=+626325.145805063: ☑️ agreed by zyguan.
  • 2024-06-24 11:10:54.372508018 +0000 UTC m=+631580.857996850: ☑️ agreed by cfzjywxk.

@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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 ti-chi-bot bot added the approved label Jun 24, 2024
@ti-chi-bot ti-chi-bot bot merged commit 7704785 into pingcap:master Jun 24, 2024
37 checks passed
@lance6716 lance6716 mentioned this pull request Jul 1, 2024
Closed
@D3Hunter D3Hunter mentioned this pull request Jul 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@windtalker windtalker windtalker left review comments

@YuJuncen YuJuncen YuJuncen approved these changes

@tangenta tangenta tangenta approved these changes

@zyguan zyguan zyguan approved these changes

@cfzjywxk cfzjywxk cfzjywxk approved these changes

Assignees
No one assigned
Labels
approved lgtm ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Batch load regions when building tikv coprocessor tasks
6 participants
@you06 @windtalker @cfzjywxk @zyguan @tangenta @YuJuncen