[GitHub] [zookeeper] kezhuw commented on a diff in pull request #1868: ZOOKEEPER-4303: Allow configuring client port to 0

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

kezhuw commented on code in PR #1868:
URL: https://github.com/apache/zookeeper/pull/1868#discussion_r902369176


##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java:
##########
@@ -427,7 +427,7 @@ public void parseProperties(Properties zkProp) throws IOException, ConfigExcepti
             dataLogDir = dataDir;
         }
 
-        if (clientPort == 0) {
+        if (clientPort == null) {

Review Comment:
   This is a behavior change but I think it make sense. Prior to this, "clientPortAddress" and `"clientPort": 0` is not allowed. It is better to allow bind to 0 with specified local address.



##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/embedded/ZookeeperServerEmbeddedTest.java:
##########
@@ -95,4 +100,31 @@ public void testStart() throws Exception {
 
     }
 
+    @Test
+    public void testBindPortZero() throws Exception {
+        final Properties configZookeeper = new Properties();
+        final ZooKeeperServerEmbedded.ZookKeeperServerEmbeddedBuilder builder = ZooKeeperServerEmbedded.builder()
+            .baseDir(baseDir)
+            .configuration(configZookeeper)
+            .exitHandler(ExitHandler.LOG_ONLY);
+
+        // Unconfigured client port will still fail
+        try (ZooKeeperServerEmbedded zkServer = builder.build()) {
+            zkServer.start();
+            assertThrows(IllegalStateException.class, new ThrowingRunnable() {
+                @Override
+                public void run() throws Throwable {
+                    zkServer.getConnectionString();
+                }
+            });
+        }
+
+        // Explicit port zero should work
+        configZookeeper.put("clientPort", "0");
+        try (ZooKeeperServerEmbedded zkServer = builder.build()) {
+            zkServer.start();
+            assertThat(zkServer.getConnectionString(), not(endsWith(":0")));
+            assertTrue(ClientBase.waitForServerUp(zkServer.getConnectionString(), 60000));
+        }
+    }

Review Comment:
   Would you mind add one more case to assert explicit "127.0.0.1:0" ? As I said above, I think it is a noticeable change.



##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java:
##########
@@ -427,7 +427,7 @@ public void parseProperties(Properties zkProp) throws IOException, ConfigExcepti
             dataLogDir = dataDir;
         }
 
-        if (clientPort == 0) {
+        if (clientPort == null) {

Review Comment:
   The implicit logic here is that: use `serverId` to lookup if not address configured. Prior to this, `0` was used as "not configured", so explicit "127.0.0.1:0" is denied.



-- 
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: notifications-unsubscribe@zookeeper.apache.org

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