You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by nkalmar <gi...@git.apache.org> on 2018/06/26 11:28:11 UTC
[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 <nk...@...>
Date: 2018-06-26T11:27:13Z
ZOOKEEPER-2873 abort startup on invalid ports
----
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Posted by enixon <gi...@git.apache.org>.
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
Posted by nkalmar <gi...@git.apache.org>.
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
Posted by enixon <gi...@git.apache.org>.
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 issue #549: ZOOKEEPER-2873 abort startup on invalid ports
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/549
Itt introduces test failures in org.apache.zookeeper.test.ReconfigTest.testUnspecifiedClientAddress, I'm working on it!
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Posted by enixon <gi...@git.apache.org>.
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
Posted by asfgit <gi...@git.apache.org>.
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
Posted by nkalmar <gi...@git.apache.org>.
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
Posted by enixon <gi...@git.apache.org>.
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
Posted by enixon <gi...@git.apache.org>.
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
Posted by enixon <gi...@git.apache.org>.
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
Posted by nkalmar <gi...@git.apache.org>.
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 issue #549: ZOOKEEPER-2873 abort startup on invalid ports
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/549
Committed to master and 3.5 branches.
Thanks @nkalmar !
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Posted by tamaashu <gi...@git.apache.org>.
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 issue #549: ZOOKEEPER-2873 abort startup on invalid ports
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/549
Doesn't the merge script do a squash?
---
[GitHub] zookeeper pull request #549: ZOOKEEPER-2873 abort startup on invalid ports
Posted by enixon <gi...@git.apache.org>.
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
Posted by nkalmar <gi...@git.apache.org>.
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
Posted by enixon <gi...@git.apache.org>.
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)`?
---