Skip to content

KAFKA-18637: Fix max connections per ip and override reconfigurations - #19099

Merged
clolov merged 1 commit into
apache:trunkfrom
azhar2407:max_connections
Mar 10, 2025
Merged

KAFKA-18637: Fix max connections per ip and override reconfigurations#19099
clolov merged 1 commit into
apache:trunkfrom
azhar2407:max_connections

Conversation

@azhar2407

Copy link
Copy Markdown
Contributor

After dynamically modifying the max.connections.per.ip configuration from 1000 to 10000, the update initially worked as expected. However, after multiple broker restarts, the value reverted to 1000, leading to numerous connection closures.

This issue occurs because the max.connections.per.ip and max.connections.per.ip.override configurations are defined using val. As a result, the values set in server.properties are assigned at initialization, and the KafkaConfig reference used by SocketServer continues to hold the initial static value.

@github-actions github-actions Bot added triage PRs from the community core Kafka Broker small Small PRs labels Mar 4, 2025

@DL1231 DL1231 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@frankvicky frankvicky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@azhar2407 : Thanks for the patch.
I have a small question.

@MethodSource(Array("getTestQuorumAndGroupProtocolParametersAll"))
def testSocketServerConfigTest(quorum: String, groupProtocol: String): Unit = {
val updatedMaxConnections = "20"
val ConnectionsIpsOverride = "1.2.3.4:1234,1.2.4.5:2345"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pardon me, why do we have a capital case for the first character of this variable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

seems like a typo. Do you think this prevents the PR merge? Sorry I am new a contributor, trying to understand if this requires a fresh revision?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No worries.
Although it's not a big issue, I suggest you fix the typo, and welcome to Kafka 😺

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix!
We need to wait for a committer to approve the CI build and review.

@github-actions github-actions Bot removed the triage PRs from the community label Mar 5, 2025
@azhar2407
azhar2407 force-pushed the max_connections branch 2 times, most recently from e4d98cd to e627cb6 Compare March 5, 2025 17:30

@clolov clolov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution!

@m1a2st m1a2st left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this PR, LGTM

@clolov
clolov merged commit 832dfa3 into apache:trunk Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Kafka Broker small Small PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants