Skip to content

executor: fix scatter region timeout issues and "show processlist" display issues#12057

Merged
sre-bot merged 4 commits intopingcap:masterfrom
zimulala:split-region
Sep 17, 2019
Merged

executor: fix scatter region timeout issues and "show processlist" display issues#12057
sre-bot merged 4 commits intopingcap:masterfrom
zimulala:split-region

Conversation

@zimulala
Copy link
Contributor

@zimulala zimulala commented Sep 6, 2019 •
edited
Loading

What problem does this PR solve?

  • Fix a single scatter region timeout does not exit

  • When processing "split table/index", the result of executing "show processlist" is as follows:

+------+------+-----------+------+---------+------+-------+------------------+
| Id   | User | Host      | db   | Command | Time | State | Info             |
+------+------+-----------+------+---------+------+-------+------------------+
|    1 | root | 127.0.0.1 | test | Query   |    0 | 2     | show processlist |
|    2 | root | 127.0.0.1 | test | Sleep   |  552 | 2     | NULL             |
+------+------+-----------+------+---------+------+-------+------------------+

What is changed and how it works?

  • Add a check when encounter timeout error. And update logs.

  • Put the operations of splitting and scattering region into Next. After the PR:

tidb> show processlist;
+------+------+-----------+------+---------+------+-------+------------------------------------------------------+
| Id   | User | Host      | db   | Command | Time | State | Info                                                 |
+------+------+-----------+------+---------+------+-------+------------------------------------------------------+
|    2 | root | 127.0.0.1 | test | Query   |    2 | 2     | split table x between (0) and (1000000) regions 1000 |
|    1 | root | 127.0.0.1 | test | Query   |    0 | 2     | show processlist                                     |
+------+------+-----------+------+---------+------+-------+------------------------------------------------------+
2 rows in set (0.00 sec)

Check List

Tests

  • Unit test

@zimulala zimulala added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Sep 6, 2019
@codecov
Copy link

codecov bot commented Sep 6, 2019 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##            master    #12057   +/-   ##
=========================================
  Coverage   81.492%   81.492%           
=========================================
  Files          457       457           
  Lines       101448    101448           
=========================================
  Hits         82672     82672           
  Misses       12971     12971           
  Partials      5805      5805

@zimulala
Copy link
Contributor Author

zimulala commented Sep 10, 2019

PTAL @crazycs520 @bb7133 @winkyao

bb7133
bb7133 reviewed Sep 10, 2019
View reviewed changes
executor/split.go
func (e *SplitIndexRegionExec) Open(ctx context.Context) error {
return e.splitIndexRegion(ctx)
func (e *SplitIndexRegionExec) Open(ctx context.Context) (err error) {
e.splitIdxKeys, err = e.getSplitIdxKeys()
Copy link
Member

@bb7133 bb7133 Sep 10, 2019

Choose a reason for hiding this comment

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

Why moving getSplitIdxKeys() to Open?

Copy link
Contributor Author

@zimulala zimulala Sep 17, 2019

Choose a reason for hiding this comment

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

Because we do some checks in getSplitIdxKeys, if put it to Next some test cases will be failed and I think best to put it before Next.

crazycs520
crazycs520 reviewed Sep 10, 2019
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 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
bb7133 approved these changes Sep 17, 2019
View reviewed changes
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

@zimulala zimulala added the status/can-merge Indicates a PR has been approved by a committer. label Sep 17, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 17, 2019

/run-all-tests

@zimulala zimulala added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 17, 2019
@sre-bot sre-bot merged commit fa675ca into pingcap:master Sep 17, 2019
@zimulala zimulala deleted the split-region branch September 17, 2019 04:01
@zimulala
Copy link
Contributor Author

zimulala commented Sep 17, 2019

Related PR #6861

zimulala added a commit to zimulala/tidb that referenced this pull request Sep 24, 2019
@zimulala
executor: fix scatter region timeout issues and "show processlist" di…
041b8e1 
…splay issues (pingcap#12057)
@zimulala zimulala mentioned this pull request Sep 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@bb7133 bb7133 bb7133 approved these changes

Assignees

No one assigned

Labels

sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zimulala @sre-bot @bb7133 @crazycs520