[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user asfgit closed the pull request at: https://github.com/apache/zookeeper/pull/549 ---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198607891
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerConfigTest.java ---
@@ -103,6 +103,23 @@ public void testCustomSSLAuth()
}
}
+/**
+ * Test case for https://issues.apache.org/jira/browse/ZOOKEEPER-2873
+ */
+@Test
+public void testSamePortConfiguredForClientAndElection() throws
IOException, ConfigException {
+QuorumPeerConfig quorumPeerConfig = new QuorumPeerConfig();
+try {
+Properties zkProp = getDefaultZKProperties();
+zkProp.setProperty("server.1", "localhost:2888:2888");
+quorumPeerConfig.parseProperties(zkProp);
+fail("ConfigException is expected");
+} catch (ConfigException ce) {
--- End diff --
I agree that your test as written is more exact, my proposal is more about
future proofing the test so it doesn't get hung up on the exact language of the
exception thrown.
I will happily defer the point to someone with more ZooKeeper experience
about which is more in the project's style.
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198607295
--- Diff: src/java/test/org/apache/zookeeper/test/ReconfigTest.java ---
@@ -801,10 +801,12 @@ private void testPortChangeToBlockedPort(boolean
testLeader) throws Exception {
@Test
public void testUnspecifiedClientAddress() throws Exception {
- int[] ports = new int[3];
- for (int port : ports) {
- port = PortAssignment.unique();
- }
+ int[] ports = {
+PortAssignment.unique(),
+PortAssignment.unique(),
+PortAssignment.unique()
+ };
+
--- End diff --
agreed, good catch
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198609106
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
@@ -259,6 +259,11 @@ public QuorumServer(long sid, String addressStr)
throws ConfigException {
throw new ConfigException("Address unresolved: " +
serverParts[0] + ":" + serverParts[2]);
}
+if(addr.getPort() == electionAddr.getPort()) {
+throw new ConfigException(
+"Client and election port must be different!
Please update the configuration file on server." + sid);
+}
+
--- End diff --
Double checked and you're right, all the other constructors for this class
are only invoked in tests. Agreed that my proposal is a "nice to have" at best.
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user tamaashu commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198532231
--- Diff: src/java/test/org/apache/zookeeper/test/ReconfigTest.java ---
@@ -801,10 +801,12 @@ private void testPortChangeToBlockedPort(boolean
testLeader) throws Exception {
@Test
public void testUnspecifiedClientAddress() throws Exception {
- int[] ports = new int[3];
- for (int port : ports) {
- port = PortAssignment.unique();
- }
+ int[] ports = {
+PortAssignment.unique(),
+PortAssignment.unique(),
+PortAssignment.unique()
+ };
+
--- End diff --
@nkalmar good catch
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198408262
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerConfigTest.java ---
@@ -103,6 +103,23 @@ public void testCustomSSLAuth()
}
}
+/**
+ * Test case for https://issues.apache.org/jira/browse/ZOOKEEPER-2873
+ */
+@Test
+public void testSamePortConfiguredForClientAndElection() throws
IOException, ConfigException {
--- End diff --
Thanks, I will fix this with an amend commit (I don't want a whole commit
in history just for this change)
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198407883
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
@@ -259,6 +259,11 @@ public QuorumServer(long sid, String addressStr)
throws ConfigException {
throw new ConfigException("Address unresolved: " +
serverParts[0] + ":" + serverParts[2]);
}
+if(addr.getPort() == electionAddr.getPort()) {
+throw new ConfigException(
+"Client and election port must be different!
Please update the configuration file on server." + sid);
+}
+
--- End diff --
Good question. Well, as what I can see it is only used for test, and it is
called programmatically, so not from reading a config. The intention here was
to make sure that ZK is not misconfigured.
So I'm open to adding it there also, just my initial thinking was it is not
necessary.
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198405834
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerConfigTest.java ---
@@ -103,6 +103,23 @@ public void testCustomSSLAuth()
}
}
+/**
+ * Test case for https://issues.apache.org/jira/browse/ZOOKEEPER-2873
+ */
+@Test
+public void testSamePortConfiguredForClientAndElection() throws
IOException, ConfigException {
+QuorumPeerConfig quorumPeerConfig = new QuorumPeerConfig();
+try {
+Properties zkProp = getDefaultZKProperties();
+zkProp.setProperty("server.1", "localhost:2888:2888");
+quorumPeerConfig.parseProperties(zkProp);
+fail("ConfigException is expected");
+} catch (ConfigException ce) {
--- End diff --
That will only verify the exception was thrown. I am also checking what is
the error message. We can use expected, but I think this is more "To the point"
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198404697
--- Diff: src/java/test/org/apache/zookeeper/test/ReconfigTest.java ---
@@ -801,10 +801,12 @@ private void testPortChangeToBlockedPort(boolean
testLeader) throws Exception {
@Test
public void testUnspecifiedClientAddress() throws Exception {
- int[] ports = new int[3];
- for (int port : ports) {
- port = PortAssignment.unique();
- }
+ int[] ports = {
+PortAssignment.unique(),
+PortAssignment.unique(),
+PortAssignment.unique()
+ };
+
--- End diff --
Yes, they will fail otherwise with the introduced changes - all the ports
were null. Also, this is just bad java code, the for does nothing, while the
intention was clearly to set the ports for a unique number.
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198339591
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
@@ -259,6 +259,11 @@ public QuorumServer(long sid, String addressStr)
throws ConfigException {
throw new ConfigException("Address unresolved: " +
serverParts[0] + ":" + serverParts[2]);
}
+if(addr.getPort() == electionAddr.getPort()) {
--- End diff --
really happy we're adding this safeguard!
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198338704
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
@@ -259,6 +259,11 @@ public QuorumServer(long sid, String addressStr)
throws ConfigException {
throw new ConfigException("Address unresolved: " +
serverParts[0] + ":" + serverParts[2]);
}
+if(addr.getPort() == electionAddr.getPort()) {
+throw new ConfigException(
+"Client and election port must be different!
Please update the configuration file on server." + sid);
+}
+
--- End diff --
Does it make sense to apply a similar change to the
`public QuorumServer(long id, InetSocketAddress addr,
InetSocketAddress electionAddr, InetSocketAddress clientAddr, LearnerType
type) {`
constructor below?
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198338264
--- Diff: src/java/test/org/apache/zookeeper/test/ReconfigTest.java ---
@@ -801,10 +801,12 @@ private void testPortChangeToBlockedPort(boolean
testLeader) throws Exception {
@Test
public void testUnspecifiedClientAddress() throws Exception {
- int[] ports = new int[3];
- for (int port : ports) {
- port = PortAssignment.unique();
- }
+ int[] ports = {
+PortAssignment.unique(),
+PortAssignment.unique(),
+PortAssignment.unique()
+ };
+
--- End diff --
is it necessary to make this change?
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198338214
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerConfigTest.java ---
@@ -103,6 +103,23 @@ public void testCustomSSLAuth()
}
}
+/**
+ * Test case for https://issues.apache.org/jira/browse/ZOOKEEPER-2873
+ */
+@Test
+public void testSamePortConfiguredForClientAndElection() throws
IOException, ConfigException {
--- End diff --
nit - ConfigException is unused
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/549#discussion_r198339563
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerConfigTest.java ---
@@ -103,6 +103,23 @@ public void testCustomSSLAuth()
}
}
+/**
+ * Test case for https://issues.apache.org/jira/browse/ZOOKEEPER-2873
+ */
+@Test
+public void testSamePortConfiguredForClientAndElection() throws
IOException, ConfigException {
+QuorumPeerConfig quorumPeerConfig = new QuorumPeerConfig();
+try {
+Properties zkProp = getDefaultZKProperties();
+zkProp.setProperty("server.1", "localhost:2888:2888");
+quorumPeerConfig.parseProperties(zkProp);
+fail("ConfigException is expected");
+} catch (ConfigException ce) {
--- End diff --
Instead of the try-catch paradigm, can we use `@Test(expected =
ConfigException.class)`?
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
GitHub user nkalmar opened a pull request: https://github.com/apache/zookeeper/pull/549 ZOOKEEPER-2873 abort startup on invalid ports You can merge this pull request into a Git repository by running: $ git pull https://github.com/nkalmar/zookeeper ZOOKEEPER-2873 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/549.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #549 commit 3d747c08fba8170765802ce40fd5e3d7734a3fa1 Author: Norbert Kalmar Date: 2018-06-26T11:27:13Z ZOOKEEPER-2873 abort startup on invalid ports ---
