lhotari commented on a change in pull request #10961: URL: https://github.com/apache/pulsar/pull/10961#discussion_r655138308
########## File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java ########## @@ -1360,10 +1360,13 @@ public ShutdownService getShutdownService() { return shutdownService; } + public static String advertisedAddress(ServiceConfiguration config) { Review comment: Thanks for clarifying it @315157973 . However, this isn't clarified in the code. The original problem remains: there's the `PulsarService.advertisedAddress` method which is almost the same as `org.apache.pulsar.broker.ServiceConfigurationUtils.getAppliedAdvertisedAddress` , but there's no description in the code why these are different and why there's a need to have 2 separate methods. It would be good to summarize this conversation in the javadocs & code comments since that's what maintainers of the code will be reading. Perhaps @Anonymitaet could help in documenting this? ########## File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java ########## @@ -1360,10 +1360,13 @@ public ShutdownService getShutdownService() { return shutdownService; } + public static String advertisedAddress(ServiceConfiguration config) { Review comment: The difference between `PulsarService.advertisedAddress` and `org.apache.pulsar.broker.ServiceConfigurationUtils.getAppliedAdvertisedAddress` hasn't been addressed in the code comments. That was my original feedback. It seems wrong to have duplication in code between PulsarService and ServiceConfigurationUtils without a clear distinction. There would be no need to clarify this duplication if there's a way to eliminate the duplication. Do you have a way to resolve the duplication with further refactoring? ########## File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java ########## @@ -1360,10 +1360,13 @@ public ShutdownService getShutdownService() { return shutdownService; } + public static String advertisedAddress(ServiceConfiguration config) { Review comment: > How about add such an method in `ServiceConfigurationUtils`? > > getAppliedAdvertisedAddress(ServiceConfiguration configuration, boolean ignoreAdvertisedListener) :+1: I think this could resolve the issue. Documenting the reason and meaning of `ignoreAdvertisedListener` in javadoc would be helpful. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org