[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config

[GitBox <gi...@apache.org>]

codelipenghui commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r895184770


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
                 ClientConfigurationData conf =
                         ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
 
-                conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
-                                ? this.brokerServiceUrlTls : this.brokerServiceUrl);
-                conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
-                conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+                boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();

Review Comment:
   Looks like the reason why we can't use the `isTlsEnabled` here is that we can have tls port and non-tls port for a broker? It's better to add a description here to clarify it.
   
   And, please also add the compatibility explanation for the change. Or do we need to consider the compatibility issue? Since users can upgrade to a new version broker but with the old version config file.
   
   The document of `brokerClientTlsEnabled` also needs to update since we also use broker clients for system topics.
   
   



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org