[GitHub] [pulsar] lhotari commented on pull request #11681: [Broker] Support disabling non-TLS service ports

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

lhotari commented on pull request #11681:
URL: https://github.com/apache/pulsar/pull/11681#issuecomment-900130782


   > I am not sure how does your PR solve #11548. From what I can see, you are basically changing the function worker service and tests. There is nothing related to brokers. Can you clarify how does your PR fix #11548?
   
   @sijie This is the first issue to hit with "pulsar standalone":
   ```
   12:11:04.314 [main] ERROR org.apache.pulsar.broker.PulsarService - Failed to start Pulsar service: No value present
   java.util.NoSuchElementException: No value present
   	at java.util.Optional.get(Optional.java:148) ~[?:?]
   	at org.apache.pulsar.broker.loadbalance.NoopLoadManager.start(NoopLoadManager.java:55) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.broker.PulsarService.startLoadManagementService(PulsarService.java:1047) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:751) [pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.PulsarStandalone.start(PulsarStandalone.java:296) [pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:131) [pulsar-broker.jar:2.9.0-SNAPSHOT]
   12:11:04.314 [main] ERROR org.apache.pulsar.PulsarStandaloneStarter - Failed to start pulsar service.
   org.apache.pulsar.broker.PulsarServerException: java.util.NoSuchElementException: No value present
   	at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:796) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.PulsarStandalone.start(PulsarStandalone.java:296) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.PulsarStandaloneStarter.main(PulsarStandaloneStarter.java:131) [pulsar-broker.jar:2.9.0-SNAPSHOT]
   Caused by: java.util.NoSuchElementException: No value present
   	at java.util.Optional.get(Optional.java:148) ~[?:?]
   	at org.apache.pulsar.broker.loadbalance.NoopLoadManager.start(NoopLoadManager.java:55) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.broker.PulsarService.startLoadManagementService(PulsarService.java:1047) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
   	at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:751) ~[pulsar-broker.jar:2.9.0-SNAPSHOT]
   	... 2 more
   
   ```
   
   Perhaps the PR description is currently confusing. In PR #11548, the problem is that when you set non-tls ports to an empty value, exceptions will happen. I tested with Pulsar development version by building Pulsar locally and appending these settings to `standalone.conf`:
   ```
   brokerServicePort=
   brokerServicePortTls=6651
   webServicePort=
   webServicePortTls=8443
   brokerClientTlsEnabled=true
   tlsEnabled=true
   tlsAllowInsecureConnection=true
   tlsCertificateFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.cert.pem
   tlsKeyFilePath=pulsar-broker/src/test/resources/authentication/tls-http/broker.key-pk8.pem
   tlsTrustCertsFilePath=pulsar-broker/src/test/resources/authentication/tls-http/ca.cert.pem
   ``
   and then starting Pulsar with `./bin/pulsar standalone`. I simply kept iterating until the problems were fixed. 
   
   I also added unit tests to verify that an empty port value gets converted to Optional.empty. The unit test for TLS without non-TLS ports enabled is also added to verify that in unit tests. That's the reason to add these tests. I added these tests before starting to work on testing it manually with Pulsar standalone.
   
   The reason why the change to Function Worker service is required is that "useTls" setting is deprecated in configuration:
   https://github.com/apache/pulsar/blob/047fb6a3ca0cf9c137a5597339e20247d4c61cdf/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java#L357-L360
   The same applies to "tlsEnabled" in broker.conf. [It has been marked as deprecated](https://github.com/apache/pulsar/blob/ddeae659be1f80f428fdb8c82c7a9a0c931c81fe/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L1020-L1021).
   tlsEnabled was deprecated by PR #2988 many years ago.
   At first I was testing without setting tlsEnabled=true and that's why I hit the issue and needed to make the changes.
   
   @sijie I didn't record all of the exceptions that happen without the changes and what this PR fixes. Is that necessary for accepting this PR?


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