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

tikv:move split region request to pd #459

Merged
merged 3 commits into from
Apr 14, 2022
Merged

Conversation

qidi1
Copy link
Contributor

@qidi1 qidi1 commented Mar 31, 2022

Signed-off-by: qidi1 1083369179@qq.com

What problem does this PR solve?

Issue Number: tikv/pd/issues/4095
Proposal PR: pingcap/tidb/pull/33664
The original region split and scatter are handled by tikv and pd respectively, in order to improve efficiency, the split is also changed to be handled and controlled by PD.

What is changed and how it works?

In SplitRegions function,Using split api provide by PD instead of TiKV

Code changes based on this work
Thanks to hzh0435

Check List

Tests

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

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

@disksing
Copy link
Collaborator

disksing commented Apr 1, 2022

would you like to explain why do you want to move it to using PD?

@qidi1
Copy link
Contributor Author

qidi1 commented Apr 2, 2022

would you like to explain why do you want to move it to using PD?

we are working in this issue,In Tikv,In tikv, it does not have the ability to check the current condition of the region, leading to the problem of unstable splitting. We move it to pd which having the global state of the region will reduce the probability of the problem in the above issue

@qidi1
Copy link
Contributor Author

qidi1 commented Apr 2, 2022

would you like to explain why do you want to move it to using PD?

I updated the description of the PR,PTAL

@disksing
Copy link
Collaborator

disksing commented Apr 6, 2022

It seems that the integration tests (run with tikv) are failed.

@qidi1 qidi1 force-pushed the move_split_to_pd branch 2 times, most recently from 890e414 to a3ed4bb Compare April 7, 2022 09:18
@qidi1
Copy link
Contributor Author

qidi1 commented Apr 7, 2022

Sorry, I can't run integration tests locally,Because the tidb package is not available to me

@qidi1
Copy link
Contributor Author

qidi1 commented Apr 7, 2022

Sorry, I can't run integration tests locally,Because the tidb package is not available to me

I change pd.RegionSplit like tikv.RegionSplit,please try to run it

@qidi1 qidi1 force-pushed the move_split_to_pd branch 2 times, most recently from 292a2c1 to e91f54a Compare April 11, 2022 12:40
Signed-off-by: qidi1 <1083369179@qq.com>
@disksing
Copy link
Collaborator

Hi @qidi1 , would like to fix the lint warnings? (some of them can be fixed by merging master)

tikv/split_region.go Outdated Show resolved Hide resolved
tikv/split_region.go Show resolved Hide resolved
@disksing
Copy link
Collaborator

Hi, it is nearly mergable! but there is still a lint warning:

Error: S1011: should replace loop with `regionIDs = append(regionIDs, resp.RegionsId...)` (gosimple)

Signed-off-by: qidi1 <1083369179@qq.com>
@qidi1
Copy link
Contributor Author

qidi1 commented Apr 13, 2022

Hi, it is nearly mergable! but there is still a lint warning:

Error: S1011: should replace loop with `regionIDs = append(regionIDs, resp.RegionsId...)` (gosimple)

OK, I have changed

@disksing
Copy link
Collaborator

Thanks @qidi1

@disksing disksing merged commit aeec0c0 into tikv:master Apr 14, 2022
disksing added a commit to oh-my-tidb/client-go that referenced this pull request Apr 24, 2022
disksing added a commit to oh-my-tidb/client-go that referenced this pull request Apr 24, 2022
This reverts commit aeec0c0.

Signed-off-by: disksing <i@disksing.com>
zkkxu pushed a commit to zkkxu/tikv-client-go that referenced this pull request Apr 24, 2022
crazycs520 added a commit to crazycs520/client-go that referenced this pull request Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants