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.

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

KREST-4977 Allow limiting the number of active connections. #315

Merged
merged 2 commits into from Jun 13, 2022

Conversation

dimitarndimitrov
Copy link
Member

This change enables configurable on startup connection limiting per server and per connector based on Jetty's ConnectionLimit listener (see its Javadoc and this Jetty issue about its usage).

That mechanism allows limiting the number of active connections (already opened + currently being opened) connections as with long-running connections, like with Kafka REST's Produce v3 streams, rate limits don't guarantee that we won't have an accumulation of connections past what the server can withstand.

This has been tested locally via the aforementioned Kafka REST Produce v3 streams.
After setting server.connection.limit=3 and opening 3 Produce v3 streams, a subsequent Get Topics call did not establish a connection until the first opened Produce v3 stream timed out.
In the command output below notice the Time Spent column, indicating that this call took slightly less than the 30 second timeout which killed the first Produce v3 stream and opened a space for a new connection:

ddimitrov@maconfluent kafka-rest % curl http://localhost:8082/v3/clusters/lkc-kjg39m/topics/ | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   121    0   121    0     0      4      0 --:--:--  0:00:26 --:--:--    27
{
  "kind": "KafkaTopicList",
  "metadata": {
    "self": "http://localhost:8082/v3/clusters/lkc-kjg39m/topics",
    "next": null
  },
  "data": []
}

@dimitarndimitrov dimitarndimitrov self-assigned this Jun 9, 2022
@dimitarndimitrov dimitarndimitrov requested a review from a team as a code owner June 9, 2022 15:32
+ "limit is reached further connections will not be accepted until the number of active "
+ "connections goes below that limit again. Active connections here means all already "
+ "opened connections plus all connections that are in the process of being accepted. "
+ "If the limit is set to a non-positive number, no limit is applied. Default is 0.";
Copy link
Member

Choose a reason for hiding this comment

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

Is this quite right? Looking at the setting code above, there is no limit set if the default of 0 is used too (not just if a negative number is used)

Copy link
Member

Choose a reason for hiding this comment

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

Or I guess it technically is correct if 0 is considered non-positive.... But I think that requires a little processing when reading the line that I think people will miss.

Ideally connector tests, as well as combined server and connector
tests should be added as well.
Copy link
Contributor

@rigelbm rigelbm left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +544 to +551
int serverConnectionLimit = config.getServerConnectionLimit();
if (serverConnectionLimit > 0) {
addBean(new ConnectionLimit(serverConnectionLimit, getServer()));
}
int connectorConnectionLimit = config.getConnectorConnectionLimit();
if (connectorConnectionLimit > 0) {
addBean(new ConnectionLimit(connectorConnectionLimit, connectors.toArray(new Connector[0])));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Both these configurations are global the whole server?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as far as I understand. I am fairly happy with the amount of testing I have put into this, but one thing that I would have liked to have checked, but didn't have the time to, would have been to confirm the multi-app scenario.

@dimitarndimitrov dimitarndimitrov merged commit f20467b into confluentinc:master Jun 13, 2022
@srpanwar-confluent
Copy link
Contributor

@dimitarndimitrov the master builds for the last few days are failing. Can you verify this change is not the root cause?https://confluentinc.atlassian.net/browse/MMA-12033

cc @abhijeet2096-confluent @kkeshav-confluent

@dimitarndimitrov
Copy link
Member Author

@dimitarndimitrov the master builds for the last few days are failing. Can you verify this change is not the root cause?https://confluentinc.atlassian.net/browse/MMA-12033

cc @abhijeet2096-confluent @kkeshav-confluent

Thanks, @srpanwar-confluent - one test from this change is failing on Jenkins. I'll take a look to see if I can find a nice way to fix it, although I feel that I might have to use a crude solution like increasing timeouts. I'll add you as a reviewer to the PR for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants