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
