MINOR: add lower case lister name integration test #19932
Conversation
Signed-off-by: PoAn Yang <payang@apache.org>
github-actions
bot
added
core
...mmon/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java
Show resolved
Hide resolved
| ) { cluster => | ||
| cluster.format() | ||
| cluster.startup() | ||
| TestUtils.waitUntilTrue(() => cluster.brokers().get(0).brokerState == BrokerState.RUNNING, |
There was a problem hiding this comment.
What we want to test is to add a lower-case listener name to the broker configuration and then ensure it works. Therefore, could you please ensure the "external" is NOT converted to upper case when creating the embedded kafka? For example, you could check the value of listeners or inter.broker.listener.name within the KafkaConfig.
There was a problem hiding this comment.
Thanks for the suggestion. I add some new assertions for listeners, inter.broker.listener.name, and listener.security.protocol.map.
Signed-off-by: PoAn Yang <payang@apache.org>
|
@FrankYang0529 could you please add unit test to ensure the lower case does not work for controller listener name? see #19191 (comment) |
Signed-off-by: PoAn Yang <payang@apache.org>
|
@chia7712 I add a unit test |
| BrokerServer broker = entry.getValue(); | ||
| ListenerName listenerName = nodes.brokerListenerName(); | ||
| int port = broker.boundPort(listenerName); | ||
| int port = broker.boundPort(ListenerName.normalised(listenerName.value())); |
There was a problem hiding this comment.
Could you please add comments to explain the behavior? Also, the controller listener name needs comment too.
There was a problem hiding this comment.
the controller listener name needs comment too.
line#578
There was a problem hiding this comment.
Updated it. Thanks.
Signed-off-by: PoAn Yang <payang@apache.org>
2 participants
In KIP-1143, it
deprecated Endpoint#listenerName and removed
org.apache.kafka.network.EndPoint. Certain parts of the code depend on
listener name normalization. We should add a test to make sure there is
no regression.
Followup: https:
//github.com//pull/19191#issuecomment-2939855317 Reviewers:
Chia-Ping Tsai chia7712@gmail.com