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

Revert "topsql: enable topsql feature by default (#33195)" #33493

Merged
merged 6 commits into from
Mar 28, 2022
Merged

Revert "topsql: enable topsql feature by default (#33195)" #33493

merged 6 commits into from
Mar 28, 2022

Conversation

crazycs520
Copy link
Contributor

This reverts commit 44cf398.

What problem does this PR solve?

Issue Number: close #33492

Problem Summary: Disable TopSQL by default since the performance regression is too large

What is changed and how it works?

Check List

Tests

Release note

None

@crazycs520 crazycs520 requested a review from a team as a code owner March 28, 2022 03:19
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Mar 28, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • breeswish
  • zhongzc

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 ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 28, 2022
@crazycs520
Copy link
Contributor Author

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 28, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/81027f2ac6522592592712c2bb459776dbb48ed2

@ngaut
Copy link
Member

ngaut commented Mar 28, 2022

I remember someone is try to do more optimizations to fix this issue. Is it works?

@crazycs520
Copy link
Contributor Author

I remember someone is try to do more optimizations to fix this issue. Is it works?

The current optimization effect is not significant for the time being.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 28, 2022
@mornyx
Copy link
Contributor

mornyx commented Mar 28, 2022

LGTM

@breezewish
Copy link
Member

I remember someone is try to do more optimizations to fix this issue. Is it works?

@ngaut The current performance regression for Sysbench PointSelect is still notable (>5%) so that we have decided to not enable it by default for now.

Luckily,

  • In more complicated cases like TPCC and TPCH the regression is small, so that user is likely to not meet such high regressions in real world.
  • Users can enable or disable the Top SQL feature on-demand in the UI or via SQL.
  • For "Windows Task Manager" in Windows and "Monitor" in MacOS, user only calls it out when needed, i.e. the system is slow. So that we think Top SQL can act similarly: enable on-demand when cluster is in high load.

For reasons above, we will use v6.0.0 with default disabled as the first iteration. It will guide us whether we should do further optimizations in future, or just enable it by default later.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 28, 2022
@breezewish
Copy link
Member

Please also roll in @bb7133 to review whether this PR rollback is a safe change.

qw4990
qw4990 approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

/merge

@crazycs520
Copy link
Contributor Author

/run-all-tests

@breezewish
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: a41a11e

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 28, 2022
@breezewish
Copy link
Member

/test

@breezewish
Copy link
Member

/run-unit-test

@breezewish
Copy link
Member

/merge

@breezewish
Copy link
Member

/run-unit-test

@crazycs520
Copy link
Contributor Author

/merge

@ti-chi-bot ti-chi-bot merged commit 5c931aa into pingcap:master Mar 28, 2022
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Mar 28, 2022
@crazycs520 @ti-srebot
cherry pick pingcap#33493 to release-6.0
af63eee 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Mar 28, 2022
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-6.0 in PR #33522

ti-chi-bot pushed a commit that referenced this pull request Mar 29, 2022
@ti-srebot
Revert "topsql: enable topsql feature by default (#33195)" (#33493) (#…
6f28ac4 
…33522)

close #33492
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish approved these changes

@qw4990 qw4990 qw4990 approved these changes

@zhongzc zhongzc zhongzc approved these changes

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 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.

Disable TopSQL by default since the performance regression is too large
9 participants
@crazycs520 @ti-chi-bot @sre-bot @ngaut @mornyx @breezewish @ti-srebot @qw4990 @zhongzc