JooHyukKim commented on issue #19866: URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1582407793
My apologies for the delay, @tisonkun! 🙌 Here is my analysis and proposal in semi-PIP style, referencing https://github.com/apache/pulsar/issues/16691, thought it will be easier to understand. Let me know what you think 👍🏻👍🏻 ## Proposal : Warning on failed verifcation ### Analysis : current Implementation - Done about handling of `BrokerServiceUrl` vs `ServiceUrl`. - Below are the most obvious places that I could find with the help of IntelliJ IDE. - Further investigation(searching) shall be done only if we agree necessary. | Method Location | Behavior | URL | |-----------------------------------------|----------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `WebSocketService.createClientInstance` | `BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used. | [Link 1](https://github.com/apache/pulsar/blob/c39f7bb123a995538fc8fa9a54b572c73fee39db/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/WebSocketService.java#L220-L231) | | `BrokerService.getReplicationClient` | `BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used. | [Link 2](https://github.com/apache/pulsar/blob/c39f7bb123a995538fc8fa9a54b572c73fee39db/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1350-L1351) | | `NameSpaceService.getNamespaceClient` | `BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used. | [Link 3](https://github.com/apache/pulsar/blob/c39f7bb123a995538fc8fa9a54b572c73fee39db/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L1480-L1491) | | `TopicLookupBase.lookupTopicAsync` | Verifies at least one of `BrokerServiceUrl` and `ServiceUrl` is not blank. | [Link 4](https://github.com/apache/pulsar/blob/aa7decc5b75894e864f3c5962c5cffad255abf25/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L239-L249) | ....with this search result and considering additional checks like using `tlsEnable` are spread around, **throwing an exception on failed URL verification** seems to cost more than what it's worth. ### Proposal & Conculsion > although, if BrokerServiceUrl takes precedence over ServiceUrl, perhaps a warning is enough. That said, a clear model > of these options is somehow missing. I also think a warning is a viable starting option at this point. Like you said, we still do not have a clear model of these options. Even before clarifying the configuration model in the future, users can already enjoy the benefit from a proper warning message on misconfiguration. -- 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]
