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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
