KAFKA-18275: Restarting broker in testing should use the same port #18381
+58
−24
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
triage
peterxcli
commented
Jan 3, 2025
Comment on lines
+52
to
+63
|
|
||
| if (socketChannel != null) { | ||
| if (socketChannel.isOpen()) { | ||
| return socketChannel; | ||
| } | ||
| // bind the server socket with same port | ||
| socketAddress = new InetSocketAddress(socketAddress.getHostString(), socketChannel.socket().getLocalPort()); | ||
| socketChannel = ServerSocketFactory.INSTANCE.openServerSocket( | ||
| listenerName, | ||
| socketAddress, | ||
| listenBacklogSize, | ||
| recvBufferSize); |
There was a problem hiding this comment.
This PR depends #18337
peterxcli
commented
Jan 4, 2025
Comment on lines
-1434
to
-1450
| private void useFixedBrokerPort() throws IOException { | ||
| // Find a free port and use it in the Kafka broker's listeners config. We can't use port 0 in the listeners | ||
| // config to get a random free port because in this test we want to stop the Kafka broker and then bring it | ||
| // back up and listening on the same port in order to verify that the Connect cluster can re-connect to Kafka | ||
| // and continue functioning normally. If we were to use port 0 here, the Kafka broker would most likely listen | ||
| // on a different random free port the second time it is started. Note that we can only use the static port | ||
| // because we have a single broker setup in this test. | ||
| int listenerPort; | ||
| try (ServerSocket s = new ServerSocket(0)) { | ||
| listenerPort = s.getLocalPort(); | ||
| } | ||
| brokerProps.put(SocketServerConfigs.LISTENERS_CONFIG, String.format("EXTERNAL://localhost:%d,CONTROLLER://localhost:0", listenerPort)); | ||
| connectBuilder | ||
| .numBrokers(1) | ||
| .brokerProps(brokerProps); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove this to pass the following two tests.
| Module | Test | Message | Time |
|---|---|---|---|
| ConnectWorkerIntegrationTest | testBrokerCoordinator() | org.opentest4j.AssertionFailedError: The Kafka cluster used in this test was not able to start successfully in time. If no recent changes have altered the behavior of Kafka brokers or clients, and this error is not occurring frequently, it is probably the result of the testing machine being temporarily overloaded and can be safely ignored. | 62.84s |
| ConnectWorkerIntegrationTest | testRequestTimeouts() | org.opentest4j.AssertionFailedError: The Kafka cluster used in this test was not able to start successfully in time. If no recent changes have altered the behavior of Kafka brokers or clients, and this error is not occurring frequently, it is probably the result of the testing machine being temporarily overloaded and can be safely ignored. | 60.51s |
https://github.com/apache/kafka/actions/runs/12595111592?pr=18381
But I don't really know why, investigating...
There was a problem hiding this comment.
oh, EmbeddedConnect uses EmbeddedKafkaCluster, and EmbeddedKafkaCluster uses KafkaClusterTestKit to build cluster, and since the brokers has assigned fixed ports in KafkaClusterTestKit, there is no need to modify the LISTENERS_CONFIG for them
peterxcli
commented
Jan 4, 2025
Comment on lines
-250
to
-251
| useFixedBrokerPort(); | ||
|
|
peterxcli
commented
Jan 4, 2025
Comment on lines
-816
to
-817
| useFixedBrokerPort(); | ||
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable brokers to restart with the same port in ClusterTest.
Tests
In the newly added test
testBrokerRestart, if we don't have:to preallocate the ports for brokers, it would failed with following logs and error: