nodece commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r895192277
##########
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.
The `isTlsEnabled` has been deprecated and used for the broker, not the
client. The `brokerClient` prefix is used for client/admin in config file.
Currently, the `getAdminClient()` has the same check, link to
https://github.com/apache/pulsar/pull/16014/files#diff-8db8be87611ed21e4a80eb15b8749729d23a49a835cb0401c8c72e5225241b21R1461,
we should keep config.
> 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.
OK, we should update this description to be cleaner.
--
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]
