Skip to content

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

Merged
merged 16 commits into from
Apr 30, 2025

Conversation

FrankYang0529
Copy link
Member

@FrankYang0529 FrankYang0529 commented Mar 12, 2025

  • 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 [email protected], TaiJuWu
[email protected], Jhen-Yung Hsu [email protected], TengYao
Chi [email protected], Ken Huang [email protected], Bagda
Parth , Kuan-Po Tseng [email protected]

@FrankYang0529 FrankYang0529 changed the title KAFKA-18760: Deprecate Optional<String> and return String from public EndPoint#listenerName (wip) KAFKA-18760: Deprecate Optional<String> and return String from public Endpoint#listener (wip) Apr 15, 2025
@FrankYang0529 FrankYang0529 changed the title KAFKA-18760: Deprecate Optional<String> and return String from public Endpoint#listener (wip) KAFKA-18760: Deprecate Optional<String> and return String from public Endpoint#listener Apr 15, 2025
Copy link
Member

@chia7712 chia7712 left a 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
Copy link
Member

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
Copy link
Member

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) {
Copy link
Member

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;
Copy link
Member

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?

Copy link
Collaborator

@TaiJuWu TaiJuWu left a 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.

@chia7712
Copy link
Member

@FrankYang0529 please fix the conflicts

@chia7712
Copy link
Member

@FrankYang0529 please fix the conflicts

@chia7712
Copy link
Member

@FrankYang0529 please rebase code to include the fix

Copy link
Member

@brandboat brandboat left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@frankvicky frankvicky merged commit 81881de into apache:trunk Apr 30, 2025
26 checks passed
@FrankYang0529 FrankYang0529 deleted the KAFKA-18760 branch May 1, 2025 07:24
shmily7829 pushed a commit to shmily7829/kafka that referenced this pull request May 7, 2025
… 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 <[email protected]>, TaiJuWu
 <[email protected]>, Jhen-Yung Hsu <[email protected]>, TengYao
 Chi <[email protected]>, Ken Huang <[email protected]>, Bagda
 Parth  , Kuan-Po Tseng <[email protected]>

---------

Signed-off-by: PoAn Yang <[email protected]>
@omkreddy
Copy link
Contributor

omkreddy commented Jun 4, 2025

@FrankYang0529 Can we add a basic integration test that uses lowercase listener names?
Since certain parts of the code depend on listener name normalization, having a test would help to catch any regressions.

@FrankYang0529
Copy link
Member Author

Yes, I will create an integration test for it. Thanks.

@FrankYang0529
Copy link
Member Author

Created a followup PR: #19932

@chia7712
Copy link
Member

Since certain parts of the code depend on listener name normalization, having a test would help to catch any regressions.

@omkreddy Do you mean the listener name SHOULD be normalization? There is a path using controller.listener.names which is NOT normalized. see https://github.com/apache/kafka/blob/3.9/core/src/main/scala/kafka/server/KafkaConfig.scala#L856

Do you think that is a bug?

@FrankYang0529
Copy link
Member Author

In my testing, only controller.listener.names cannot be lowercase. Other fields like listeners, advertised.listeners, and listener.security.protocol.map can accept lowercase controller listener name. If we don't want to change the behavior, I recommend to add validation to KafkaConfig, so users don't get unclear message like following.

requirement failed: The listeners config must only contain KRaft controller listeners from controller.listener.names when process.roles=controller

They should get clear message like:

requirement failed: controller.listener.names doesn't allow lowercase

chia7712 pushed a commit that referenced this pull request Jun 17, 2025
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants