-
Notifications
You must be signed in to change notification settings - Fork 14.5k
KAFKA-18760: Deprecate Optional<String> and return String from public Endpoint#listener #19191
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
Conversation
9a88650
to
576b3df
Compare
… EndPoint#listenerName Signed-off-by: PoAn Yang <payang@apache.org>
576b3df
to
5a57f0b
Compare
Signed-off-by: PoAn Yang <payang@apache.org>
Signed-off-by: PoAn Yang <payang@apache.org>
Signed-off-by: PoAn Yang <payang@apache.org>
Signed-off-by: PoAn Yang <payang@apache.org>
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.
@FrankYang0529 thanks for this patch
/** | ||
* @deprecated Since 4.1. Use {@link #listener()} instead. This function will be removed in 5.0. | ||
*/ | ||
@Deprecated |
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.
@Deprecated(since = "4.1", forRemoval = true)
*/ | ||
@Deprecated |
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.
@Deprecated(since = "4.1", forRemoval = true)
public class Endpoint { | ||
|
||
private final String listenerName; | ||
private final SecurityProtocol securityProtocol; | ||
private final String host; | ||
private final int port; | ||
|
||
public static String parseListenerName(String connectionString) { |
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 seems only KafkaConfig
uses this helper. Could you please move it to the KafkaConfig
instead of leaving it in this public APIs?
* Returns the listener name of this endpoint. | ||
*/ | ||
public String listener() { | ||
return listenerName; |
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.
How about renaming it to listenerName
for consistency?
Signed-off-by: PoAn Yang <payang@apache.org>
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, thanks for this patch.
@FrankYang0529 please fix the conflicts |
@FrankYang0529 please fix the conflicts |
@FrankYang0529 please rebase code to include the fix |
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, thanks!
… Endpoint#listener (apache#19191) * Deprecate org.apache.kafka.common.Endpoint#listenerName. * Add org.apache.kafka.common.Endpoint#listener to replace org.apache.kafka.common.Endpoint#listenerName. * Replace org.apache.kafka.network.EndPoint with org.apache.kafka.common.Endpoint. * Deprecate org.apache.kafka.clients.admin.RaftVoterEndpoint#name * Add org.apache.kafka.clients.admin.RaftVoterEndpoint#listener to replace org.apache.kafka.clients.admin.RaftVoterEndpoint#name Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, TaiJuWu <tjwu1217@gmail.com>, Jhen-Yung Hsu <jhenyunghsu@gmail.com>, TengYao Chi <frankvicky@apache.org>, Ken Huang <s7133700@gmail.com>, Bagda Parth , Kuan-Po Tseng <brandboat@gmail.com> --------- Signed-off-by: PoAn Yang <payang@apache.org>
@FrankYang0529 Can we add a basic integration test that uses lowercase listener names? |
Yes, I will create an integration test for it. Thanks. |
Created a followup PR: #19932 |
@omkreddy Do you mean the listener name SHOULD be normalization? There is a path using Do you think that is a bug? |
In my testing, only
They should get clear message like:
|
In [KIP-1143](https://cwiki.apache.org/confluence/x/LwqWF), 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>
org.apache.kafka.common.Endpoint#listenerName.
org.apache.kafka.common.Endpoint.
replace org.apache.kafka.clients.admin.RaftVoterEndpoint#name
Reviewers: Chia-Ping Tsai chia7712@gmail.com, TaiJuWu
tjwu1217@gmail.com, Jhen-Yung Hsu jhenyunghsu@gmail.com, TengYao
Chi frankvicky@apache.org, Ken Huang s7133700@gmail.com, Bagda
Parth , Kuan-Po Tseng brandboat@gmail.com