EronWright commented on a change in pull request #12389:
URL: https://github.com/apache/pulsar/pull/12389#discussion_r730145131



##########
File path: 
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/validator/MultipleListenerValidator.java
##########
@@ -39,10 +39,10 @@
 
     /**
      * Validate the configuration of `advertisedListeners`, 
`internalListenerName`.
-     * 2. the listener name in `advertisedListeners` must not duplicate.
-     * 3. user can not assign same 'host:port' to different listener.
-     * 4. if `internalListenerName` is absent, the first `listener` in the 
`advertisedListeners` will be the `internalListenerName`.
-     * 5. if pulsar do not specify `brokerServicePortTls`, should only contain 
one entry of `pulsar://` per listener name.
+     * 1. the listener name in `advertisedListeners` must not duplicate.
+     * 2. user can not assign same 'host:port' to different listener.
+     * 3. if `internalListenerName` is absent, the first `listener` in the 
`advertisedListeners` will be the `internalListenerName`.
+     * 4. if pulsar do not specify `brokerServicePortTls`, should only contain 
one entry of `pulsar://` per listener name.

Review comment:
       Note that (4) is obsolete.
   
   May I suggest:
   1. `advertisedListeners` consists of a comma-separated list of endpoints.
   2. Each endpoint consists of a listener name and an associated address 
(`listener_name:scheme://host:port`).
   3. A listener name may be repeated to define both a non-TLS and a TLS 
endpoint.
   4. Duplicate definitions are disallowed.
   5. If `internalListenerName` is absent, set it to the first listener defined 
in `advertisedListeners`.

##########
File path: 
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/validator/MultipleListenerValidator.java
##########
@@ -39,10 +39,10 @@
 
     /**
      * Validate the configuration of `advertisedListeners`, 
`internalListenerName`.
-     * 2. the listener name in `advertisedListeners` must not duplicate.
-     * 3. user can not assign same 'host:port' to different listener.
-     * 4. if `internalListenerName` is absent, the first `listener` in the 
`advertisedListeners` will be the `internalListenerName`.
-     * 5. if pulsar do not specify `brokerServicePortTls`, should only contain 
one entry of `pulsar://` per listener name.
+     * 1. the listener name in `advertisedListeners` must not duplicate.
+     * 2. user can not assign same 'host:port' to different listener.
+     * 3. if `internalListenerName` is absent, the first `listener` in the 
`advertisedListeners` will be the `internalListenerName`.
+     * 4. if pulsar do not specify `brokerServicePortTls`, should only contain 
one entry of `pulsar://` per listener name.

Review comment:
       Note that (4) is obsolete.
   
   May I suggest:
   1. `advertisedListeners` consists of a comma-separated list of endpoints.
   2. Each endpoint consists of a listener name and an associated address 
(`listener:scheme://host:port`).
   3. A listener name may be repeated to define both a non-TLS and a TLS 
endpoint.
   4. Duplicate definitions are disallowed.
   5. If `internalListenerName` is absent, set it to the first listener defined 
in `advertisedListeners`.




-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to