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

br: use common scan-split-scatter logic #52433

Merged
merged 9 commits into from Apr 10, 2024

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented Apr 9, 2024

What problem does this PR solve?

Issue Number: ref #51533

Problem Summary:

What changed and how does it work?

Check List

Tests

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.

None

Signed-off-by: lance6716 <lance6716@gmail.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 9, 2024
@lance6716
Copy link
Contributor Author

/test pull-br-integration-test

@ti-chi-bot ti-chi-bot bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 9, 2024
Copy link

tiprow bot commented Apr 9, 2024

Hi @lance6716. 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/test-infra repository.

Copy link

tiprow bot commented Apr 9, 2024

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

In response to this:

/test pull-br-integration-test

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/test-infra repository.

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Merging #52433 (6505cac) into master (a0b5861) will increase coverage by 2.8886%.
Report is 15 commits behind head on master.
The diff coverage is 79.3103%.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #52433        +/-   ##
================================================
+ Coverage   72.1392%   75.0279%   +2.8886%     
================================================
  Files          1467       1467                
  Lines        426844     427329       +485     
================================================
+ Hits         307922     320616     +12694     
+ Misses        99686      86516     -13170     
- Partials      19236      20197       +961     
Flag Coverage Δ
integration 50.0133% <67.2413%> (?)
unit 71.0056% <41.3793%> (-0.0074%) ⬇️

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

Components Coverage Δ
dumpling 53.9957% <ø> (ø)
parser ∅ <ø> (∅)
br 59.4671% <78.9473%> (+18.3921%) ⬆️

Signed-off-by: lance6716 <lance6716@gmail.com>
@lance6716
Copy link
Contributor Author

/ok-to-test

@ti-chi-bot ti-chi-bot bot added ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 9, 2024
Signed-off-by: lance6716 <lance6716@gmail.com>
@lance6716
Copy link
Contributor Author

/test pull-br-integration-test

Copy link

tiprow bot commented Apr 9, 2024

@lance6716: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test fast_test_tiprow
  • /test tidb_parser_test

Use /test all to run all jobs.

In response to this:

/test pull-br-integration-test

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/test-infra repository.

Signed-off-by: lance6716 <lance6716@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2024
@lance6716
Copy link
Contributor Author

/test pull-br-integration-test

Copy link

tiprow bot commented Apr 9, 2024

@lance6716: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test fast_test_tiprow
  • /test tidb_parser_test

Use /test all to run all jobs.

In response to this:

/test pull-br-integration-test

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/test-infra repository.

@lance6716 lance6716 changed the title [WIP]br: use common scan-split-scatter logic br: use common scan-split-scatter logic Apr 9, 2024
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 9, 2024
@@ -125,14 +106,14 @@ func (rs *RegionSplitter) executeSplitByRanges(
roughSortedSplitKeys = append(roughSortedSplitKeys, sortedKeys[curRegionIndex])
}
if len(roughSortedSplitKeys) > 0 {
if err := rs.executeSplitByKeys(ctx, splitContext, scanStartKey, roughSortedSplitKeys); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

scanStartKey is the start key. but sortedKeys are all end keys. if didn't scan with start key will cause error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't need to split the scanStartKey so theoretically we don't need to care about it. For the error I should have fixed in

// we need to find the regions that contain the split keys. However, the scan
// region API accepts a key range [start, end) where end key is exclusive, and if
// sortedSplitKeys length is 1, we scan region may return empty result. So we
// increase the end key a bit. If the end key is on the region boundaries, it
// will be skipped by getSplitKeysOfRegions.
scanStart := codec.EncodeBytesExt(nil, sortedSplitKeys[0], c.isRawKv)
lastKey := kv.Key(sortedSplitKeys[len(sortedSplitKeys)-1])
scanEnd := codec.EncodeBytesExt(nil, lastKey.Next(), c.isRawKv)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: I fixed a bug and add tests in 59e6f0f

Copy link
Contributor

@3pointer 3pointer Apr 9, 2024

Choose a reason for hiding this comment

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

Okay, We need make sure not use same key as start/end key to scan regions. or we will get an error from PD. I assume here is no chance to get a slice with only one nil/"" value in it.

Signed-off-by: lance6716 <lance6716@gmail.com>
Signed-off-by: lance6716 <lance6716@gmail.com>
Signed-off-by: lance6716 <lance6716@gmail.com>
Copy link

ti-chi-bot bot commented Apr 9, 2024

@lance6716: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-br-integration-test 6505cac link true /test pull-br-integration-test

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@@ -409,7 +338,7 @@ func (helper *LogSplitHelper) splitRegionByPoints(
}

helper.pool.ApplyOnErrorGroup(helper.eg, func() error {
newRegions, errSplit := regionSplitter.splitAndScatterRegions(ctx, region, splitPoints, false)
newRegions, errSplit := regionSplitter.splitAndScatterRegions(ctx, region, splitPoints)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now it can directly call regionSplitter.client.SplitWaitAndScatter and the function splitAndScatterRegions can be removed.

Copy link

ti-chi-bot bot commented Apr 10, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-04-10 03:24:53.823554522 +0000 UTC m=+1019155.351095085: ☑️ agreed by Leavrth.
  • 2024-04-10 03:43:36.899534537 +0000 UTC m=+1020278.427075083: ☑️ agreed by 3pointer.

@lance6716
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Apr 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, lance6716, Leavrth

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 Apr 10, 2024
@ti-chi-bot ti-chi-bot bot merged commit 25da0bc into pingcap:master Apr 10, 2024
20 of 21 checks passed
3AceShowHand pushed a commit to 3AceShowHand/tidb that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test release-note-none size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants