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

executor/split: return split result when do split region and refine split timeout logic. #11259

Merged
merged 15 commits into from
Jul 23, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Jul 15, 2019

What problem does this PR solve?

  • return a split result when do split region
  • refine split timeout logic.

The old split region timeout logic was return error when timeout. But return error is not suitable. Because after TiDB send split and scatter request to PD, we should think the request must be executed successfully. The cost time of split and scatter request finish depends on the TiKV/PD cluster whether busy.

mysql>set @@tidb_wait_split_region_timeout=10;
Query OK, 0 rows affected
Time: 0.000s
mysql>split table t between (100000) and (200000) regions 10;
+--------------------+----------------------+
| TOTAL_SPLIT_REGION | SCATTER_FINISH_RATIO |
+--------------------+----------------------+
| 9                  | 0.444444444444       |
+--------------------+----------------------+
1 row in set
Time: 10.427s

What is changed and how it works?

Check List

Tests

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

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch

@crazycs520
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #11259 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11259   +/-   ##
===========================================
  Coverage   81.4875%   81.4875%           
===========================================
  Files           423        423           
  Lines         91020      91020           
===========================================
  Hits          74170      74170           
  Misses        11542      11542           
  Partials       5308       5308

executor/executor_test.go Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
@sre-bot sre-bot mentioned this pull request Jul 19, 2019
executor/split.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
executor/split.go Show resolved Hide resolved
planner/core/planbuilder.go Outdated Show resolved Hide resolved
if err != nil {
if isCtxDone(ctxWithTimeout) {
// Do not break here for checking remain region scatter finished with a very short back off time.
// Imagine this situation, we split region 1,2,3, and timeout on wait region 1 scatter,
Copy link
Member

Choose a reason for hiding this comment

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

IMHO the comment is still not good enough... would you please get help from some other colleagues, like i18n team?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

store/tikv/split_region.go Outdated Show resolved Hide resolved
executor/split.go Show resolved Hide resolved
crazycs520 and others added 5 commits July 23, 2019 11:35
Co-Authored-By: bb7133 <bb7133@gmail.com>
Co-Authored-By: bb7133 <bb7133@gmail.com>
executor/split.go Outdated Show resolved Hide resolved
executor/split.go Outdated Show resolved Hide resolved
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133
Copy link
Member

bb7133 commented Jul 23, 2019

/run-all-tests

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests

@bb7133 bb7133 merged commit 3b6d2f4 into pingcap:master Jul 23, 2019
@winkyao
Copy link
Contributor

winkyao commented Jul 27, 2019

/run-cherry-picker

2 similar comments
@you06
Copy link
Contributor

you06 commented Jul 28, 2019

/run-cherry-picker

@you06
Copy link
Contributor

you06 commented Jul 28, 2019

/run-cherry-picker

@sre-bot
Copy link
Contributor

sre-bot commented Jul 28, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Jul 28, 2019

cherry pick to release-3.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants