sysvar, config: introduce tidb_connection_concurrency_limit as an instance scope sysvar. #34662

CbcWestwolf · 2022-05-15T07:39:19Z

What problem does this PR solve?

Issue Number: close #34583

Problem Summary:

For token-limit there is a use-case to make it online configurable. And the name of token-limit is too tied to an implementation, not what the feature does.

What is changed and how it works?

Introducing tidb_connection_concurrency_limit as a sysvar and a config item in [instance] section.

Check List

Tests

Unit test (added in previous UT)
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

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

None

ti-chi-bot · 2022-05-15T07:39:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

morgo

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.

sre-bot · 2022-05-15T07:50:23Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/2985c9ad767a03a68d22ee0fa5d8ef5c5a0dd345

morgo · 2022-05-16T15:06:01Z

server/server.go

-		concurrentLimiter: NewTokenLimiter(cfg.TokenLimit),
+		concurrentLimiter: NewTokenLimiter(uint(cfg.Instance.ConnectionConcurrencyLimit)),


I think this will just be passed to server/ when the server starts?

If so, it will need to be refactored, since making changes to the sysvar won't apply until restart (which is a problem for instance vars, since the values are lost on restart too).

… change_token_limit

morgo · 2022-05-19T03:05:07Z

@CbcWestwolf Please fix linter issues:


[2022-05-18T15:07:18.743Z] linting

[2022-05-18T15:07:18.743Z]   ✘  https://revive.run/r#exported  exported method Server.UpdateTokenLimiter should have comment or be unexported  

[2022-05-18T15:07:18.743Z]   server/server.go:164:1

[2022-05-18T15:07:18.743Z] 

[2022-05-18T15:07:18.743Z]   ✘  https://revive.run/r#exported  exported method TokenLimiter.Resize should have comment or be unexported  

[2022-05-18T15:07:18.743Z]   server/tokenlimiter.go:46:1

[2022-05-18T15:07:18.743Z] 

[2022-05-18T15:07:18.743Z] ✘ 2 problems (2 errors, 0 warnings)

[2022-05-18T15:07:18.743Z] 

[2022-05-18T15:07:18.743Z] Errors:

[2022-05-18T15:07:18.743Z]   2  exported

morgo · 2022-05-19T03:23:05Z

server/conn.go

@@ -1324,6 +1324,7 @@ func (cc *clientConn) dispatch(ctx context.Context, data []byte) error {
 		}

 		cc.server.releaseToken(token)
+		cc.server.UpdateTokenLimiter()


I'm curious why update here and not in the SetGlobal func?

CbcWestwolf · 2022-05-19T03:28:14Z

@CbcWestwolf Please fix linter issues:

Thanks, it is updated. PTAL.
And I'm adding some tests to check the UpdateTokenLimiter. I'm afraid that this PR introduce a great performance regression due to the mutex in TokenLimiter, do you think so?

morgo · 2022-05-19T03:39:31Z

Thanks, it is updated. PTAL.
And I'm adding some tests to check the UpdateTokenLimiter. I'm afraid that this PR introduce a great performance regression due to the mutex in TokenLimiter, do you think so?

I was actually wondering the same thing. I think we might need to test it.

The other option is to refactor to use an atomic for the limit, and not use a resizer. It probably does change quite a bit of code though, so I'm not sure if its better.

morgo

LGTM, please fix conflicts

… change_token_limit

ti-chi-bot · 2022-08-12T19:09:59Z

@CbcWestwolf: PR needs rebase.

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.

codecov · 2023-07-22T09:49:44Z

Codecov Report

Merging #34662 (2985c9a) into master (fbbae46) will increase coverage by 11.9434%.
Report is 4476 commits behind head on master.
The diff coverage is 67.6878%.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #34662         +/-   ##
=================================================
+ Coverage   61.8719%   73.8153%   +11.9434%     
=================================================
  Files           891        990         +99     
  Lines        284074     313741      +29667     
=================================================
+ Hits         175762     231589      +55827     
+ Misses        94536      67553      -26983     
- Partials      13776      14599        +823

tiprow · 2024-02-27T14:23:55Z

@CbcWestwolf: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	`2985c9a`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

ti-chi-bot · 2024-08-28T11:28:19Z

@CbcWestwolf: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-mysql-client-test	`2985c9a`	link	true	`/test pull-mysql-client-test`
pull-integration-ddl-test	`2985c9a`	link	true	`/test pull-integration-ddl-test`
idc-jenkins-ci-tidb/check_dev	`2985c9a`	link	true	`/test check-dev`
idc-jenkins-ci-tidb/mysql-test	`2985c9a`	link	true	`/test mysql-test`
idc-jenkins-ci-tidb/build	`2985c9a`	link	true	`/test build`
idc-jenkins-ci-tidb/check_dev_2	`2985c9a`	link	true	`/test check-dev2`
idc-jenkins-ci-tidb/unit-test	`2985c9a`	link	true	`/test unit-test`
pull-br-integration-test	`2985c9a`	link	true	`/test pull-br-integration-test`
pull-lightning-integration-test	`2985c9a`	link	true	`/test pull-lightning-integration-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

sysvar,config: introduce tidb_connection_concurrency_limit

a064adf

CbcWestwolf requested a review from a team as a code owner May 15, 2022 07:39

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 May 15, 2022

hawkingrei and others added 2 commits May 15, 2022 19:21

Merge branch 'master' into change_token_limit

d33a28b

Merge branch 'master' into change_token_limit

7b8a7e4

morgo reviewed May 16, 2022

View reviewed changes

CbcWestwolf added 4 commits May 17, 2022 11:06

Fix

4c71453

Merge branch 'change_token_limit' of github.com:CbcWestwolf/tidb into…

58d3385

… change_token_limit

Merge branch 'master' of github.com:pingcap/tidb into change_token_limit

6038f07

Introduce TokenLimiter.Resize()

ae1df77

Fix check_dev

0ad5567

morgo reviewed May 19, 2022

View reviewed changes

CbcWestwolf added 2 commits May 19, 2022 19:06

Refactor

e9ba4a9

Merge branch 'master' of github.com:pingcap/tidb into change_token_limit

f25f0b6

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2022

Fix

51c4be7

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2022

CbcWestwolf added 2 commits May 19, 2022 19:55

Fix

155f981

Merge branch 'master' into change_token_limit

d635b76

morgo self-requested a review May 25, 2022 23:46

morgo approved these changes May 25, 2022

View reviewed changes

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

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2022

morgo mentioned this pull request May 25, 2022

Expose config options as instance scoped sysvars #34960

Open

Merge branch 'master' of github.com:pingcap/tidb into change_token_limit

1d35815

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2022

CbcWestwolf and others added 2 commits June 13, 2022 15:05

Merge branch 'master' of github.com:pingcap/tidb into change_token_limit

8d73590

Merge branch 'master' into change_token_limit

ae1e2b9

CbcWestwolf marked this pull request as draft June 23, 2022 08:37

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 23, 2022

CbcWestwolf added 2 commits August 4, 2022 17:47

Merge branch 'change_token_limit' of github.com:CbcWestwolf/tidb into…

0065286

… change_token_limit

Merge branch 'master' of github.com:pingcap/tidb into change_token_limit

3a88c94

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 4, 2022

Merge branch 'master' of github.com:pingcap/tidb into change_token_limit

2985c9a

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2022

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2022

ti-chi-bot bot deleted a comment from ti-chi-bot Dec 19, 2023

CbcWestwolf closed this Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sysvar, config: introduce tidb_connection_concurrency_limit as an instance scope sysvar. #34662

sysvar, config: introduce tidb_connection_concurrency_limit as an instance scope sysvar. #34662

CbcWestwolf commented May 15, 2022

ti-chi-bot commented May 15, 2022 •

edited

Loading

sre-bot commented May 15, 2022 •

edited

Loading

morgo May 16, 2022

morgo commented May 19, 2022

morgo May 19, 2022

CbcWestwolf commented May 19, 2022

morgo commented May 19, 2022

morgo left a comment

ti-chi-bot commented Aug 12, 2022

codecov bot commented Jul 22, 2023 •

edited

Loading

tiprow bot commented Feb 27, 2024

ti-chi-bot bot commented Aug 28, 2024

		concurrentLimiter: NewTokenLimiter(cfg.TokenLimit),
		concurrentLimiter: NewTokenLimiter(uint(cfg.Instance.ConnectionConcurrencyLimit)),

sysvar, config: introduce tidb_connection_concurrency_limit as an instance scope sysvar. #34662

sysvar, config: introduce tidb_connection_concurrency_limit as an instance scope sysvar. #34662

Conversation

CbcWestwolf commented May 15, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 15, 2022 • edited Loading

sre-bot commented May 15, 2022 • edited Loading

morgo May 16, 2022

Choose a reason for hiding this comment

morgo commented May 19, 2022

morgo May 19, 2022

Choose a reason for hiding this comment

CbcWestwolf commented May 19, 2022

morgo commented May 19, 2022

morgo left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Aug 12, 2022

codecov bot commented Jul 22, 2023 • edited Loading

Codecov Report

tiprow bot commented Feb 27, 2024

ti-chi-bot bot commented Aug 28, 2024

ti-chi-bot commented May 15, 2022 •

edited

Loading

sre-bot commented May 15, 2022 •

edited

Loading

codecov bot commented Jul 22, 2023 •

edited

Loading