*: add global/instance variable to config top sql #24934

crazycs520 · 2021-05-27T11:05:10Z

Signed-off-by: crazycs [email protected]

Trace issue: #24875

What problem does this PR solve?

This PR adds the following instance/global variable to config Top SQL feature:

	// TiDBEnableTopSQL indicates whether the top SQL is enabled.
	TiDBEnableTopSQL = "tidb_enable_top_sql"

	// TiDBTopSQLAgentAddress indicates the top SQL agent address.
	TiDBTopSQLAgentAddress = "tidb_top_sql_agent_address"

	// TiDBTopSQLPrecisionSeconds indicates the top SQL precision seconds.
	TiDBTopSQLPrecisionSeconds = "tidb_top_sql_precision_seconds"

	// TiDBTopSQLMaxStatementCount indicates the max number of statements been collected.
	TiDBTopSQLMaxStatementCount = "tidb_top_sql_max_statement_count"

What is changed and how it works?

N/A

Related changes

PR to update pingcap/docs/pingcap/docs-cn: WIP?

Check List

Tests

Unit test

Side effects

N/A

Release note

No release note

Signed-off-by: crazycs <[email protected]>

…-config

crazycs520 · 2021-05-27T11:29:34Z

/run-all-tests

…-config

crazycs520 · 2021-05-28T02:37:45Z

/run-all-tests

executor/set.go

domain/sysvar_cache.go

Signed-off-by: crazycs <[email protected]>

morgo · 2021-05-28T03:22:20Z

config/config.go

@@ -137,7 +145,7 @@ type Config struct {
 	DelayCleanTableLock uint64      `toml:"delay-clean-table-lock" json:"delay-clean-table-lock"`
 	SplitRegionMaxNum   uint64      `toml:"split-region-max-num" json:"split-region-max-num"`
 	StmtSummary         StmtSummary `toml:"stmt-summary" json:"stmt-summary"`
-	TopSQL              TopSQL      `toml:"top-sql" json:"top-sql"`
+	TopSQL              TopSQL      `toml:"-" json:"-"`


Can you explain why the toml and json names are blanked out - is the goal to no longer specify them in toml, but instead use them in sysvars?

Because the TopSQL feature doesn't want to provide a config item in the TiDB config file, only provide the instance/global variable to control.

@breeswish PTAL

Because I think it is hard for the user to understand in this situation:

The user changes the config item in config file and restart the TiDB server, then which value takes effect? the config value or the global variable value? It will confuse the user.

As an instance config that is not placed in the config file, is it necessary to be inside the config.go?

Just for coding convenience. Of course, we can use a unique global variable to do it. Should we ?

I will add a new global variable to store the TopSQL config, and remove it from config pkg.

sessionctx/variable/sysvar.go

Signed-off-by: crazycs <[email protected]>

…-config

crazycs520 · 2021-05-28T08:30:24Z

/run-all-tests

Signed-off-by: crazycs <[email protected]>

…nto top-sql-config

crazycs520 · 2021-05-31T03:21:04Z

@morgo PTAL, thanks~~

morgo · 2021-05-31T03:48:18Z

/LGTM

ti-chi-bot · 2021-05-31T03:48:21Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

morgo
tangenta

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

crazycs520 · 2021-05-31T03:53:40Z

/run-all-tests

crazycs520 · 2021-05-31T03:53:54Z

/merge

ti-chi-bot · 2021-05-31T03:53:56Z

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

Commit hash: e86fa65

crazycs520 · 2021-05-31T06:15:08Z

/run-unit-test

crazycs520 · 2021-05-31T06:18:03Z

/run-check_dev_2

crazycs520 · 2021-05-31T06:41:57Z

/run-unit-test

crazycs520 · 2021-05-31T06:58:19Z

/run-unit-test

Signed-off-by: crazycs <[email protected]>

crazycs520 · 2021-05-31T07:07:28Z

/merge

ti-chi-bot · 2021-05-31T07:07:32Z

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

Commit hash: 38f257a

ti-chi-bot · 2021-05-31T07:07:45Z

@crazycs520: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

*: add global/instance variable to config top sql

dc18204

Signed-off-by: crazycs <[email protected]>

crazycs520 requested a review from a team as a code owner May 27, 2021 11:05

crazycs520 requested review from wshwsh12 and removed request for a team May 27, 2021 11:05

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 27, 2021

crazycs520 mentioned this pull request May 27, 2021

Implement Top-SQL feature #24875

Open

7 tasks

github-actions bot added component/config sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels May 27, 2021

crazycs520 added 2 commits May 27, 2021 19:24

fix test

4b00877

Signed-off-by: crazycs <[email protected]>

Merge branch 'master' of https://github.com/pingcap/tidb into top-sql…

2b5a7eb

…-config

Merge branch 'master' of https://github.com/pingcap/tidb into top-sql…

6522259

…-config

morgo reviewed May 28, 2021

View reviewed changes

executor/set.go Outdated Show resolved Hide resolved

domain/sysvar_cache.go Outdated Show resolved Hide resolved

address comment and remove useless code

cc0837a

Signed-off-by: crazycs <[email protected]>

morgo reviewed May 28, 2021

View reviewed changes

tangenta reviewed May 28, 2021

View reviewed changes

sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved

tangenta reviewed May 28, 2021

View reviewed changes

sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved

move top sql variable from config to variable

dd1b451

Signed-off-by: crazycs <[email protected]>

crazycs520 force-pushed the top-sql-config branch from cb50324 to 8cda431 Compare May 28, 2021 08:25

Merge branch 'master' of https://github.com/pingcap/tidb into top-sql…

fa1b5c3

…-config

crazycs520 force-pushed the top-sql-config branch from 8cda431 to fa1b5c3 Compare May 28, 2021 08:27

tangenta approved these changes May 28, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 28, 2021

tangenta requested a review from morgo May 28, 2021 08:48

crazycs520 added 3 commits May 28, 2021 16:56

Merge branch 'master' into top-sql-config

a553c05

address comment to change default value and max value

4c70e71

Signed-off-by: crazycs <[email protected]>

Merge branch 'top-sql-config' of https://github.com/crazycs520/tidb i…

0f8274c

…nto top-sql-config

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 May 31, 2021

Merge branch 'master' into top-sql-config

e86fa65

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 31, 2021

ti-chi-bot added 2 commits May 31, 2021 13:46

Merge branch 'master' into top-sql-config

3dd6ac8

Merge branch 'master' into top-sql-config

0dbcec5

Merge branch 'master' into top-sql-config

c0ac680

use serial test

38f257a

Signed-off-by: crazycs <[email protected]>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label May 31, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 31, 2021

Merge branch 'master' into top-sql-config

4b953d9

ti-chi-bot merged commit 702c86b into pingcap:master May 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add global/instance variable to config top sql #24934

*: add global/instance variable to config top sql #24934

crazycs520 commented May 27, 2021

crazycs520 commented May 27, 2021

crazycs520 commented May 28, 2021

morgo May 28, 2021

crazycs520 May 28, 2021

crazycs520 May 28, 2021

crazycs520 May 28, 2021

breezewish May 28, 2021

crazycs520 May 28, 2021

crazycs520 May 28, 2021 •

edited

Loading

crazycs520 commented May 28, 2021

crazycs520 commented May 31, 2021

morgo commented May 31, 2021

ti-chi-bot commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

ti-chi-bot commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

ti-chi-bot commented May 31, 2021

ti-chi-bot commented May 31, 2021

*: add global/instance variable to config top sql #24934

*: add global/instance variable to config top sql #24934

Conversation

crazycs520 commented May 27, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

crazycs520 commented May 27, 2021

crazycs520 commented May 28, 2021

morgo May 28, 2021

Choose a reason for hiding this comment

crazycs520 May 28, 2021

Choose a reason for hiding this comment

crazycs520 May 28, 2021

Choose a reason for hiding this comment

crazycs520 May 28, 2021

Choose a reason for hiding this comment

breezewish May 28, 2021

Choose a reason for hiding this comment

crazycs520 May 28, 2021

Choose a reason for hiding this comment

crazycs520 May 28, 2021 • edited Loading

Choose a reason for hiding this comment

crazycs520 commented May 28, 2021

crazycs520 commented May 31, 2021

morgo commented May 31, 2021

ti-chi-bot commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

ti-chi-bot commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

crazycs520 commented May 31, 2021

ti-chi-bot commented May 31, 2021

ti-chi-bot commented May 31, 2021

crazycs520 May 28, 2021 •

edited

Loading