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

builder: support EXPLAIN ANALYZE to display RU statistical information #42080

Merged
merged 15 commits into from
Mar 17, 2023
Merged

builder: support EXPLAIN ANALYZE to display RU statistical information #42080

merged 15 commits into from
Mar 17, 2023

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Mar 9, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: close #41972, #42087.

Problem Summary:

  • Support EXPLAIN ANALYZE to display RU statistical information.
  • Update the PD and client-go versions.

What is changed and how it works?

See tikv/client-go#732 for more details.

Check List

Tests

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

图片

图片

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Mar 9, 2023 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • nolouch
  • tiancaiamao

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot
Copy link
Member

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 9, 2023
@JmPotato JmPotato marked this pull request as ready for review March 10, 2023 08:09
@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 10, 2023
@JmPotato
Copy link
Member Author

/cc @glorv @tiancaiamao @nolouch

PTAL, thx.

nolouch
nolouch reviewed Mar 13, 2023
View reviewed changes
Copy link
Member

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

maybe we no need to make it visible by default. How about control it by a session variable? such as mysql has a variable like :

SET @@explain_format=DEFAULT;

@JmPotato
Copy link
Member Author

maybe we no need to make it visible by default. How about control it by a session variable? such as mysql has a variable like :

Upon examining the EXPLAIN synopsis, it appears that EXPLAIN ANALYZE is unable to utilize FORMAT = "xxx" for specifying the output format. I am uncertain if implementing a new session variable to regulate this would be appropriate. Nevertheless, given that resource control is currently enabled by default, displaying RU information at all times seems favorable to me. Perhaps we could restrict showing the information only when the resource group is not empty or set as "default". What are your thoughts?

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023
@JmPotato JmPotato requested review from nolouch and removed request for tiancaiamao and glorv March 14, 2023 06:21
tiancaiamao
tiancaiamao approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

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

LGTM, please update go.mod when the dependency is ready.

@ti-chi-bot ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 14, 2023
@JmPotato
Support EXPLAIN ANALYZE to display RU statistical information
e77ed7a 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Register RURuntimeStats after execution
fb9baea 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Update DEPS.bzl
8aa916c 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Update the PD version
37769b6 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Update go.mod
b545d79 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@JmPotato
Check the resource group name before registering
fad7503 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@ti-chi-bot ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 16, 2023
@JmPotato
Merge the upstream
be2b21a 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@glorv
Copy link
Contributor

glorv commented Mar 16, 2023

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: be2b21a

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 16, 2023
@JmPotato
Copy link
Member Author

/retest

@JmPotato
Copy link
Member Author

/test unit-test

1 similar comment
@JmPotato
Copy link
Member Author

/test unit-test

@ti-chi-bot
Copy link
Member

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

  • /test build
  • /test check-dev
  • /test check-dev2
  • /test mysql-test
  • /test unit-test

Use /test all to run all jobs.

In response to this:

/test unit-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.

@JmPotato
Copy link
Member Author

/test unit-test

@JmPotato
Fix TestExplainFor
fa2d0a9 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 16, 2023
@glorv
Copy link
Contributor

glorv commented Mar 17, 2023

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: fa2d0a9

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 17, 2023
@JmPotato
Copy link
Member Author

/test unit-test

@JmPotato
Fix TestExplainFor again
09200b6 
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 17, 2023
@JmPotato
Copy link
Member Author

/test check_dev

@ti-chi-bot
Copy link
Member

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

  • /test build
  • /test check-dev
  • /test check-dev2
  • /test mysql-test
  • /test unit-test

Use /test all to run all jobs.

In response to this:

/test check_dev

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.

@JmPotato
Copy link
Member Author

/retest

@glorv
Copy link
Contributor

glorv commented Mar 17, 2023

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 09200b6

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 17, 2023
@JmPotato
Copy link
Member Author

/test check-dev

@ti-chi-bot ti-chi-bot merged commit 55d2944 into pingcap:master Mar 17, 2023
@JmPotato JmPotato deleted the ru_runtime_stats branch March 17, 2023 06:56
iosmanthus pushed a commit to iosmanthus/tidb that referenced this pull request May 11, 2023
@nolouch @JmPotato
builder: support EXPLAIN ANALYZE to display RU statistical information (
5ea8770 
pingcap#353)

* builder: support EXPLAIN ANALYZE to display RU statistical information (pingcap#42080)
Signed-off-by: nolouch <nolouch@gmail.com>
---------

Signed-off-by: nolouch <nolouch@gmail.com>
Co-authored-by: JmPotato <ghzpotato@gmail.com>
@kevin-xianliu kevin-xianliu mentioned this pull request Sep 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Connor1996 Connor1996 Connor1996 left review comments

@nolouch nolouch nolouch approved these changes

@glorv glorv glorv approved these changes

@tiancaiamao tiancaiamao tiancaiamao approved these changes

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. 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.

Make EXPLAIN ANALYZE show RU statistics info
6 participants
@JmPotato @ti-chi-bot @glorv @tiancaiamao @nolouch @Connor1996