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: fix error tikv_cop_wait time in metric profile (#19859) #19881

Merged
merged 2 commits into from
Sep 8, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #19859 to release-4.0


Signed-off-by: crazycs520 [email protected]

What problem does this PR solve?

tikv_cop_wait metric has a type label, it has 3 values: all, schedule,snapshot. And the all total_time = schedule total time + snapshot total time.

METRICS_SCHEMA> select type,sum(value) from tikv_cop_wait_total_time group by type;
+----------+---------------------+
| type     | sum(value)          |
+----------+---------------------+
| all      | 6704.248505983651   |
| schedule | 6628.518727877801   |
| snapshot |   75.85378299487088 |
+----------+---------------------+

What is changed and how it works?

When collecting the total time of tikv_cop_wait, should ignore the all type value.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Manual test

Before

image

This PR

image

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • Fix issue of tikv_cop_wait time in metric profile.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor

/run-all-tests

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 8, 2020
@crazycs520 crazycs520 added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Sep 8, 2020
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor

/run-integration-copr-test

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 8, 2020
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 8, 2020
@crazycs520
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@bb7133,Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments.See the corresponding SIG page for more information. Related SIG: execution(slack).

@bb7133 bb7133 merged commit c97f601 into pingcap:release-4.0 Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/execution SIG execution status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants