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

planner, executor: support SQL show pump/drainer status #9456

Merged
merged 7 commits into from
Mar 4, 2019
Merged

planner, executor: support SQL show pump/drainer status #9456

merged 7 commits into from
Mar 4, 2019

Conversation

caohe
Copy link
Contributor

@caohe caohe commented Feb 25, 2019 •
edited by WangXiangUSTC
Loading

What problem does this PR solve?

Impl #9201

  1. Support SQL show pump status and show drainer status

What is changed and how it works?

Changes executor and planbuilder.
Just like binlogctl, it gets pump and drainer status from PD.

Check List

Tests

  • Manual test(start pump and drainer, then execute show pump status and show drainer status to check)

Related changes

  • Need to update the documentation

@codecov-io
Copy link

codecov-io commented Feb 25, 2019 •
edited
Loading

Codecov Report

Merging #9456 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9456      +/-   ##
==========================================
- Coverage   67.38%   67.35%   -0.03%     
==========================================
  Files         375      375              
  Lines       78746    78777      +31     
==========================================
- Hits        53063    53062       -1     
- Misses      20908    20940      +32     
  Partials     4775     4775
Impacted Files Coverage Δ
planner/core/planbuilder.go 48.75% <0%> (-0.24%) ⬇️
executor/show.go 43.56% <0%> (-1.55%) ⬇️
ddl/delete_range.go 75.13% <0%> (-4.24%) ⬇️
expression/schema.go 94.53% <0%> (+0.78%) ⬆️
store/tikv/lock_resolver.go 42.65% <0%> (+0.94%) ⬆️
executor/join.go 79.06% <0%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f16081b...345c195. Read the comment docs.

@caohe caohe changed the title planner, executor: support SQL show pump/drainer status [WIP] planner, executor: support SQL show pump/drainer status Feb 25, 2019
@caohe
Copy link
Contributor Author

caohe commented Feb 25, 2019

@WangXiangUSTC PTAL

@WangXiangUSTC
Copy link
Contributor

thanks @caohe

@WangXiangUSTC WangXiangUSTC added contribution This PR is from a community contributor. component/binlog labels Feb 26, 2019
WangXiangUSTC
WangXiangUSTC reviewed Feb 26, 2019
View reviewed changes
executor/show.go Outdated Show resolved Hide resolved
WangXiangUSTC
WangXiangUSTC reviewed Feb 26, 2019
View reviewed changes
executor/show.go Outdated Show resolved Hide resolved
@caohe caohe force-pushed the show_pump_and_drainer_status branch from ea618f7 to 4b29ef9 Compare February 26, 2019 13:42
@caohe caohe changed the title [WIP] planner, executor: support SQL show pump/drainer status planner, executor: support SQL show pump/drainer status Feb 26, 2019
WangXiangUSTC
WangXiangUSTC reviewed Feb 27, 2019
View reviewed changes
executor/show.go Outdated Show resolved Hide resolved
@tiancaiamao tiancaiamao self-requested a review February 27, 2019 02:41
@WangXiangUSTC
Copy link
Contributor

rest LGTM

@tiancaiamao
Copy link
Contributor

LGTM
Although those kinds of feature is better to be implemented as plugin in my opinion...
PTAL @jackysp

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 27, 2019
zz-jason
zz-jason previously approved these changes Feb 27, 2019
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 27, 2019
@zz-jason
Copy link
Member

/run-all-tests

zz-jason
zz-jason reviewed Feb 27, 2019
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

@caohe could you add some unit test?

@caohe
Copy link
Contributor Author

caohe commented Feb 27, 2019 •
edited
Loading

@zz-jason OK. @WangXiangUSTC and I have both tested it manually. But I don't know how to add unit tests to it. It seems that I should start/mock a pump/drainer in the test code? Or simply check the number of rows and columns in the result?

@jackysp
Copy link
Member

jackysp commented Feb 27, 2019

/run-all-tests

@zz-jason
Copy link
Member

Both "mock a pump/drainer in the test code" and "simply check the number of rows and columns in the result" are acceptable to me

@caohe caohe force-pushed the show_pump_and_drainer_status branch from 0b4dfd2 to 9631e3b Compare March 1, 2019 18:24
@caohe caohe force-pushed the show_pump_and_drainer_status branch from 48c7a31 to 378e59d Compare March 4, 2019 05:03
@caohe
Copy link
Contributor Author

caohe commented Mar 4, 2019

/run-all-tests

@WangXiangUSTC
Copy link
Contributor

/run-all-tests

@caohe
Copy link
Contributor Author

caohe commented Mar 4, 2019

I have tried a few times to add unit test for this sql clause with the help of @WangXiangUSTC . But there are some difficulties in mocking pd. So I am sorry but may be not able to add unit test this time. I will add unit test when I find a better way.

@WangXiangUSTC WangXiangUSTC merged commit e7ff050 into pingcap:master Mar 4, 2019
@caohe caohe deleted the show_pump_and_drainer_status branch March 4, 2019 14:50
WangXiangUSTC pushed a commit to WangXiangUSTC/tidb that referenced this pull request Mar 26, 2019
@caohe @WangXiangUSTC
cherry pick planner, executor: support SQL show pump/drainer status (p…
5da131f 
…ingcap#9456) and solve conflict
@WangXiangUSTC WangXiangUSTC mentioned this pull request Mar 26, 2019
Merged
zimulala pushed a commit that referenced this pull request Mar 26, 2019
@WangXiangUSTC @zimulala
cherry pick planner, executor: support SQL show pump/drainer status (#…
fbab0fd 
…9456) and solve conflict (#9896)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WangXiangUSTC WangXiangUSTC WangXiangUSTC left review comments

@zz-jason zz-jason zz-jason approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

Assignees
No one assigned
Labels
component/binlog contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@caohe @codecov-io @WangXiangUSTC @tiancaiamao @zz-jason @jackysp