-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
sessionctx/variable, executor: introduce a limit on "thread" config #28842
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
isn't 128 too low? 96-core machines are not that hard to find, and 2×96 = 192. |
I picked it because it was an existing convention, but I had the same thought as you. For an absolute upper bound it is on the low side. I had also considered 256. |
In the TiDB repository I've only found |
/run-check_dev_2 |
OK, I've changed the default to 256 instead. |
068f088
to
397eb52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried: when a user has set the concurrency as 257 in the older TiDB version and then upgrade to this version, the TiDB cluster can work correctly, no matter the effective value is 257 or 256.
I assume it will work correctly, but it is worth a trial.
Can we use |
Yes, it will work. When a sysvar is loaded from a previous version the validation func is always called (this is intentional knowing there are changes to validation cross version). The value will then be truncated to
|
I think it's better if we use static values for min/max. We can use dynamic values for defaults though. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 397eb52
|
@morgo: 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. |
What problem does this PR solve?
Problem Summary:
Currently TiDB has many sysvars which configure "threads" (goroutines). The max range of some of these values is int32-max, which is not practical. Usually a practical limit is around the number of CPU cores, or 2x the number of CPU cores.
I have picked 256 as an upper bound because there are already some thread settings which safely use this as their upper bound.
What is changed and how it works?
What's Changed:
The maximum value of several system variables has been lowered to 256 for safety.
Check List
Tests
Side effects
Technically this breaks BC in two ways:
Documentation
Release note