-
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
*: support for limiting the number of client connections #14409
Conversation
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.
LGTM
config/config.go
Outdated
@@ -477,6 +479,7 @@ var defaultConf = Config{ | |||
SplitRegionMaxNum: 1000, | |||
RepairMode: false, | |||
RepairTableList: []string{}, | |||
MaxConnections: 151, |
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.
Why 151
?
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.
It's 151 in our original code and in MySQL.
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.
The reason is historical. When the apache default was 150, and MySQL was 100, there were cases where web pages would contain too many connection errors. MySQL raised its limit to Apache's +1. It does not necessarily need to be followed literally.
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.
LGTM
This has an important behavior difference from MySQL, which is that in MySQL the In MySQL 8.0 this was extended to provide a separate admin port, which is a better implementation: https://dev.mysql.com/worklog/task/?id=12138 |
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.
Hold on.
Yes, it maybe works like the extra port in MariaDB or Percona. We could discuss it in another issue. |
The connection limit applies to a single server, I think put it into the config file is better. |
OK, it is better to use a different name, e.g. |
config/config.go
Outdated
@@ -477,6 +479,7 @@ var defaultConf = Config{ | |||
SplitRegionMaxNum: 1000, | |||
RepairMode: false, | |||
RepairTableList: []string{}, | |||
MaxServerConnections: 1024, |
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.
I think set it higher like 4096 is better for compatibility.
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.
Done. PTAL @coocood
LGTM |
/merge |
/run-all-tests |
What problem does this PR solve?
Limiting the number of connections is to control that there should not be too many connections on a single server, resulting in resource occupation. This feature is similar to max_connections in MySQL, but it only controls the current server.
What is changed and how it works?
Adds a check on the connection count when a connection is established.
Check List
Tests
Code changes
Release note