[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/488 I would like to have this change in 3.4 so I will merge after the 3.4.12 release. ---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/488 @anmolnar It looks like you are right. removing this line does not impact which files git ignores. Thanks for persisting @jason95 @asutosh936 ---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/488 @asutosh936 I agree with @anmolnar That directory exists, it is currently in version control, and I don't think any generated files actually end up there. My guess is that it is there purely for historical reasons and to contain the rcc.jj file. In other words, I don't think your change to the gitignore will impact that directory. According to the gitignore documentation (https://git-scm.com/docs/gitignore): > If the pattern does not contain a slash /, Git treats it as a shell glob pattern and checks for a match against the pathname relative to the location of the .gitignore file (relative to the toplevel of the work tree if not from a .gitignore file). Since there are not any top level files called `generated` I don't think this line in the gitignore has any impact on the repository. Am I missing something here? ---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/488 > The change is to remove 'generated' keyword from .gitignore because it prevents the dir and files under generated dir to be added to the repo. The compilation later fails due to the missing dir and files. Which files are you referring to? I don't think we have any top level `generated` directory and I also don't think we would want to add such files to the repo. Don't worry about squashing, I should be able to take care of that when it comes time to commit. ---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/488 I agree with @anmolnar. This change should be fine from what I can tell but it would be nice to have a note as to why the change was made. ---
[GitHub] zookeeper pull request #474: ZOOKEEPER-2977: Concurrency for addAuth corrupt...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/474#discussion_r173243505
--- Diff: src/java/test/org/apache/zookeeper/server/ServerCnxnTest.java ---
@@ -0,0 +1,129 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.server;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.nio.ByteBuffer;
+import java.util.List;
+
+import org.apache.jute.Record;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.ZKTestCase;
+import org.apache.zookeeper.data.Id;
+import org.apache.zookeeper.proto.ReplyHeader;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ServerCnxnTest extends ZKTestCase {
+
+/**
+ * Test getting a copy of authinfo to avoid parallel modification
impact
+ */
+@Test
+public void testServerCnxnGetAuthInfoWithCopy() throws Exception {
+MockServerCnxn serverCnxn = new MockServerCnxn();
+List authInfo = serverCnxn.getAuthInfo();
--- End diff --
nit: please fix this indentation
---
[GitHub] zookeeper issue #482: ZOOKEEPER-2962 - Removed Unused method.
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/482 Please close this PR. ---
[GitHub] zookeeper issue #482: ZOOKEEPER-2962 - Removed Unused method.
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/482 Thanks @asutosh936! ---
[GitHub] zookeeper pull request #474: ZOOKEEPER-2977: Concurrency for addAuth corrupt...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/474#discussion_r171980950
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java
---
@@ -103,4 +105,22 @@ public void testValidSelectionKey() throws Exception {
zk.close();
}
}
+
+@Test(timeout = 3)
+public void testServerCnxnGetAuthInfoWithCopy() throws Exception {
+final ZooKeeper zk = createZKClient(hostPort, 3000);
+try {
+Iterable connections =
serverFactory.getConnections();
+for (ServerCnxn serverCnxn : connections) {
+ List authInfo = serverCnxn.getAuthInfo();
+ Id id = new Id("testscheme", "test");
+ serverCnxn.addAuthInfo(id);
+ Assert.assertTrue(!authInfo.contains(id));
--- End diff --
nit: let's use assertFalse
---
[GitHub] zookeeper issue #473: ZOOKEEPER-2936 - Removed duplicate code
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/473 Thanks @asutosh936! ---
[GitHub] zookeeper pull request #476: ZOOKEEPER-2988: NPE triggered if server receive...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/476#discussion_r171972767
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java ---
@@ -1064,7 +1065,12 @@ else if
(self.getCurrentAndNextConfigVoters().contains(n.sid)) {
break;
}
} else {
-LOG.warn("Ignoring notification from non-cluster
member " + n.sid);
+if
(!self.getCurrentAndNextConfigVoters().contains(n.leader)) {
+LOG.warn("Ignoring notification for non-cluster
member sid {} from sid {}", n.leader, n.sid);
+}
+if
(!self.getCurrentAndNextConfigVoters().contains(n.sid)) {
+LOG.warn("Ignoring notification from non-cluster
member sid {}", n.sid);
--- End diff --
nit: lets say "quorum" instead of "cluster"
---
[GitHub] zookeeper pull request #476: ZOOKEEPER-2988: NPE triggered if server receive...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/476#discussion_r171970721
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -1012,4 +1012,113 @@ public void testFailedTxnAsPartOfQuorumLoss()
throws Exception {
Assert.assertNull("server " + i + " should not have /zk" +
leader, servers.zk[i].exists("/zk" + leader, false));
}
}
+
+/**
+ * Verify that a node without the leader in its view will not attempt
to connect to the leader.
+ */
+@Test
+public void testLeaderOutOfView() throws Exception {
+ClientBase.setupTestEnv();
+
+Layout layout = new PatternLayout("%d{ISO8601} [,yid:%X{myid}] -
%5p [%t:%C{1}@%L] - %m%n");
--- End diff --
It would be great if we did not specify the pattern as a literal. Take a
look at some of the other tests and see if the way they get the patternlayout
could apply here (`testElectionFraud` for example).
---
[GitHub] zookeeper pull request #476: ZOOKEEPER-2988: NPE triggered if server receive...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/476#discussion_r171972228
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -1012,4 +1012,113 @@ public void testFailedTxnAsPartOfQuorumLoss()
throws Exception {
Assert.assertNull("server " + i + " should not have /zk" +
leader, servers.zk[i].exists("/zk" + leader, false));
}
}
+
+/**
+ * Verify that a node without the leader in its view will not attempt
to connect to the leader.
+ */
+@Test
+public void testLeaderOutOfView() throws Exception {
+ClientBase.setupTestEnv();
+
+Layout layout = new PatternLayout("%d{ISO8601} [,yid:%X{myid}] -
%5p [%t:%C{1}@%L] - %m%n");
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.DEBUG);
+Logger qlogger =
Logger.getLogger("org.apache.zookeeper.server.quorum");
+qlogger.addAppender(appender);
+
+try {
+final int CLIENT_PORT_QP1 = PortAssignment.unique();
+final int CLIENT_PORT_QP2 = PortAssignment.unique();
+final int CLIENT_PORT_QP3 = PortAssignment.unique();
+
+String quorumCfgIncomplete = getUniquePortCfgForId(1) + "\n" +
getUniquePortCfgForId(2);
+String quorumCfgComplete = quorumCfgIncomplete + "\n" +
getUniquePortCfgForId(3);
+
+// Node 1 is started without the leader (3) in its config view
+MainThread q1 = new MainThread(1, CLIENT_PORT_QP1,
quorumCfgIncomplete);
+MainThread q2 = new MainThread(2, CLIENT_PORT_QP2,
quorumCfgComplete);
+MainThread q3 = new MainThread(3, CLIENT_PORT_QP3,
quorumCfgComplete);
+
+// Node 1 must be started first, before quorum is formed, to
trigger the attempted invalid connection to 3
+q1.start();
+QuorumPeer quorumPeer1 = waitForQuorumPeer(q1,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer1.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 3 started second to avoid 1 and 2 forming a quorum
before 3 starts up
+q3.start();
+QuorumPeer quorumPeer3 = waitForQuorumPeer(q3,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer3.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 2 started last, kicks off leader election
+q2.start();
+
+// Nodes 2 and 3 now form quorum and fully start. 1 attempts
to vote for 3, fails, returns to LOOKING state
+Assert.assertTrue("waiting for server 2 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP2, CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 3 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP3, CONNECTION_TIMEOUT));
+
+Assert.assertTrue(q1.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+Assert.assertTrue(q2.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.FOLLOWING);
+Assert.assertTrue(q3.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LEADING);
+
+q1.shutdown();
+q2.shutdown();
+q3.shutdown();
+
+Assert.assertTrue("waiting for server 1 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP1,
+ClientBase.CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 2 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP2,
+ClientBase.CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 3 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP3,
+ClientBase.CONNECTION_TIMEOUT));
+
+} finally {
+qlogger.removeAppender(appender);
+}
+
+// Verify that Node 1 never threw an exception
+LineNumberReader r = new LineNumberReader(new
StringReader(os.toString()));
+String line;
+boolean found = false;
+Pattern p = Pattern.compile(".*java.lang.NullPointerException.*");
+while ((line = r.readLine()) != null) {
+found = p.matcher(line).matches();
+
[GitHub] zookeeper pull request #476: ZOOKEEPER-2988: NPE triggered if server receive...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/476#discussion_r171972608
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -1012,4 +1012,113 @@ public void testFailedTxnAsPartOfQuorumLoss()
throws Exception {
Assert.assertNull("server " + i + " should not have /zk" +
leader, servers.zk[i].exists("/zk" + leader, false));
}
}
+
+/**
+ * Verify that a node without the leader in its view will not attempt
to connect to the leader.
+ */
+@Test
+public void testLeaderOutOfView() throws Exception {
+ClientBase.setupTestEnv();
+
+Layout layout = new PatternLayout("%d{ISO8601} [,yid:%X{myid}] -
%5p [%t:%C{1}@%L] - %m%n");
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.DEBUG);
+Logger qlogger =
Logger.getLogger("org.apache.zookeeper.server.quorum");
+qlogger.addAppender(appender);
+
+try {
+final int CLIENT_PORT_QP1 = PortAssignment.unique();
+final int CLIENT_PORT_QP2 = PortAssignment.unique();
+final int CLIENT_PORT_QP3 = PortAssignment.unique();
+
+String quorumCfgIncomplete = getUniquePortCfgForId(1) + "\n" +
getUniquePortCfgForId(2);
+String quorumCfgComplete = quorumCfgIncomplete + "\n" +
getUniquePortCfgForId(3);
+
+// Node 1 is started without the leader (3) in its config view
+MainThread q1 = new MainThread(1, CLIENT_PORT_QP1,
quorumCfgIncomplete);
+MainThread q2 = new MainThread(2, CLIENT_PORT_QP2,
quorumCfgComplete);
+MainThread q3 = new MainThread(3, CLIENT_PORT_QP3,
quorumCfgComplete);
+
+// Node 1 must be started first, before quorum is formed, to
trigger the attempted invalid connection to 3
+q1.start();
+QuorumPeer quorumPeer1 = waitForQuorumPeer(q1,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer1.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 3 started second to avoid 1 and 2 forming a quorum
before 3 starts up
+q3.start();
+QuorumPeer quorumPeer3 = waitForQuorumPeer(q3,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer3.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 2 started last, kicks off leader election
+q2.start();
+
+// Nodes 2 and 3 now form quorum and fully start. 1 attempts
to vote for 3, fails, returns to LOOKING state
+Assert.assertTrue("waiting for server 2 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP2, CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 3 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP3, CONNECTION_TIMEOUT));
+
+Assert.assertTrue(q1.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+Assert.assertTrue(q2.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.FOLLOWING);
+Assert.assertTrue(q3.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LEADING);
+
+q1.shutdown();
--- End diff --
is there a way we can use the existing tearDown code. So we make sure we
never leave any servers running even if an assertion fails?
---
[GitHub] zookeeper pull request #476: ZOOKEEPER-2988: NPE triggered if server receive...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/476#discussion_r171964902
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java ---
@@ -1064,7 +1065,12 @@ else if
(self.getCurrentAndNextConfigVoters().contains(n.sid)) {
break;
}
} else {
-LOG.warn("Ignoring notification from non-cluster
member " + n.sid);
+if
(!self.getCurrentAndNextConfigVoters().contains(n.leader)) {
+LOG.warn("Ignoring notification for non-cluster
member sid {} from sid {}", n.leader, n.sid);
+}
+if
(!self.getCurrentAndNextConfigVoters().contains(n.sid)) {
+LOG.warn("Ignoring notification from non-cluster
member sid {}", n.sid);
--- End diff --
lets log which member was voted for
---
[GitHub] zookeeper pull request #476: ZOOKEEPER-2988: NPE triggered if server receive...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/476#discussion_r171973588
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -1012,4 +1012,113 @@ public void testFailedTxnAsPartOfQuorumLoss()
throws Exception {
Assert.assertNull("server " + i + " should not have /zk" +
leader, servers.zk[i].exists("/zk" + leader, false));
}
}
+
+/**
+ * Verify that a node without the leader in its view will not attempt
to connect to the leader.
+ */
+@Test
+public void testLeaderOutOfView() throws Exception {
+ClientBase.setupTestEnv();
+
+Layout layout = new PatternLayout("%d{ISO8601} [,yid:%X{myid}] -
%5p [%t:%C{1}@%L] - %m%n");
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.DEBUG);
+Logger qlogger =
Logger.getLogger("org.apache.zookeeper.server.quorum");
+qlogger.addAppender(appender);
+
+try {
+final int CLIENT_PORT_QP1 = PortAssignment.unique();
+final int CLIENT_PORT_QP2 = PortAssignment.unique();
+final int CLIENT_PORT_QP3 = PortAssignment.unique();
+
+String quorumCfgIncomplete = getUniquePortCfgForId(1) + "\n" +
getUniquePortCfgForId(2);
+String quorumCfgComplete = quorumCfgIncomplete + "\n" +
getUniquePortCfgForId(3);
+
+// Node 1 is started without the leader (3) in its config view
+MainThread q1 = new MainThread(1, CLIENT_PORT_QP1,
quorumCfgIncomplete);
+MainThread q2 = new MainThread(2, CLIENT_PORT_QP2,
quorumCfgComplete);
+MainThread q3 = new MainThread(3, CLIENT_PORT_QP3,
quorumCfgComplete);
+
+// Node 1 must be started first, before quorum is formed, to
trigger the attempted invalid connection to 3
+q1.start();
+QuorumPeer quorumPeer1 = waitForQuorumPeer(q1,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer1.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 3 started second to avoid 1 and 2 forming a quorum
before 3 starts up
+q3.start();
+QuorumPeer quorumPeer3 = waitForQuorumPeer(q3,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer3.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 2 started last, kicks off leader election
+q2.start();
+
+// Nodes 2 and 3 now form quorum and fully start. 1 attempts
to vote for 3, fails, returns to LOOKING state
+Assert.assertTrue("waiting for server 2 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP2, CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 3 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP3, CONNECTION_TIMEOUT));
+
+Assert.assertTrue(q1.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+Assert.assertTrue(q2.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.FOLLOWING);
+Assert.assertTrue(q3.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LEADING);
+
+q1.shutdown();
+q2.shutdown();
+q3.shutdown();
+
+Assert.assertTrue("waiting for server 1 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP1,
+ClientBase.CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 2 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP2,
+ClientBase.CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 3 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP3,
+ClientBase.CONNECTION_TIMEOUT));
+
+} finally {
+qlogger.removeAppender(appender);
+}
+
+// Verify that Node 1 never threw an exception
+LineNumberReader r = new LineNumberReader(new
StringReader(os.toString()));
+String line;
+boolean found = false;
+Pattern p = Pattern.compile(".*java.lang.NullPointerException.*");
--- End diff --
It would be great to have a more direct way of expressing that this member
never enters the following state. Can we check the logs for `FOLLOWING` like in
`testElectionFraud`?
---
[GitHub] zookeeper pull request #476: ZOOKEEPER-2988: NPE triggered if server receive...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/476#discussion_r171972029
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -1012,4 +1012,113 @@ public void testFailedTxnAsPartOfQuorumLoss()
throws Exception {
Assert.assertNull("server " + i + " should not have /zk" +
leader, servers.zk[i].exists("/zk" + leader, false));
}
}
+
+/**
+ * Verify that a node without the leader in its view will not attempt
to connect to the leader.
+ */
+@Test
+public void testLeaderOutOfView() throws Exception {
+ClientBase.setupTestEnv();
+
+Layout layout = new PatternLayout("%d{ISO8601} [,yid:%X{myid}] -
%5p [%t:%C{1}@%L] - %m%n");
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.DEBUG);
+Logger qlogger =
Logger.getLogger("org.apache.zookeeper.server.quorum");
+qlogger.addAppender(appender);
+
+try {
+final int CLIENT_PORT_QP1 = PortAssignment.unique();
+final int CLIENT_PORT_QP2 = PortAssignment.unique();
+final int CLIENT_PORT_QP3 = PortAssignment.unique();
+
+String quorumCfgIncomplete = getUniquePortCfgForId(1) + "\n" +
getUniquePortCfgForId(2);
+String quorumCfgComplete = quorumCfgIncomplete + "\n" +
getUniquePortCfgForId(3);
+
+// Node 1 is started without the leader (3) in its config view
+MainThread q1 = new MainThread(1, CLIENT_PORT_QP1,
quorumCfgIncomplete);
+MainThread q2 = new MainThread(2, CLIENT_PORT_QP2,
quorumCfgComplete);
+MainThread q3 = new MainThread(3, CLIENT_PORT_QP3,
quorumCfgComplete);
+
+// Node 1 must be started first, before quorum is formed, to
trigger the attempted invalid connection to 3
+q1.start();
+QuorumPeer quorumPeer1 = waitForQuorumPeer(q1,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer1.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 3 started second to avoid 1 and 2 forming a quorum
before 3 starts up
+q3.start();
+QuorumPeer quorumPeer3 = waitForQuorumPeer(q3,
CONNECTION_TIMEOUT);
+Assert.assertTrue(quorumPeer3.getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+
+// Node 2 started last, kicks off leader election
+q2.start();
+
+// Nodes 2 and 3 now form quorum and fully start. 1 attempts
to vote for 3, fails, returns to LOOKING state
+Assert.assertTrue("waiting for server 2 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP2, CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 3 to start",
+ClientBase.waitForServerUp("127.0.0.1:" +
CLIENT_PORT_QP3, CONNECTION_TIMEOUT));
+
+Assert.assertTrue(q1.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LOOKING);
+Assert.assertTrue(q2.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.FOLLOWING);
+Assert.assertTrue(q3.getQuorumPeer().getPeerState() ==
QuorumPeer.ServerState.LEADING);
+
+q1.shutdown();
+q2.shutdown();
+q3.shutdown();
+
+Assert.assertTrue("waiting for server 1 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP1,
+ClientBase.CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 2 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP2,
+ClientBase.CONNECTION_TIMEOUT));
+Assert.assertTrue("waiting for server 3 down",
+ClientBase.waitForServerDown("127.0.0.1:" +
CLIENT_PORT_QP3,
+ClientBase.CONNECTION_TIMEOUT));
+
+} finally {
+qlogger.removeAppender(appender);
+}
+
+// Verify that Node 1 never threw an exception
+LineNumberReader r = new LineNumberReader(new
StringReader(os.toString()));
+String line;
+boolean found = false;
+Pattern p = Pattern.compile(".*java.lang.NullPointerException.*");
+while ((line = r.readLine()) != null) {
+found = p.matcher(line).matches();
+
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/453 Thanks @revans2. I merged this and the PR's for 3.4 and 3.5 ---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/469#discussion_r170379991 --- Diff: build.xml --- @@ -1861,4 +1861,18 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + + --- End diff -- fine with me ---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/469#discussion_r170379824 --- Diff: build.xml --- @@ -1861,4 +1861,18 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- let's replace them for the sake of consistency ---
[GitHub] zookeeper issue #458: ZOOKEEPER-2967: Add check to validate dataDir and data...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/458 @mfenes Thank you, please close this PR ---
[GitHub] zookeeper issue #459: ZOOKEEPER-2967: Add check to validate dataDir and data...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/459 @mfenes Thank you, please close this PR ---
[GitHub] zookeeper issue #468: ZOOKEEPER-2982: Re-try DNS hostname -> IP resolution i...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/468 @EronWright I left a comment in the JIRA. ---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/469#discussion_r169435681
--- Diff: build.xml ---
@@ -1861,4 +1861,18 @@
xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
+
+
--- End diff --
I found a "one-liner" for this here:
http://ant.apache.org/manual/using.html#pathshortcut
For example:
```xml
${toString:java.classpath}
```
What do you think?
---
[GitHub] zookeeper pull request #469: ZOOKEEPER-2983: Print the classpath when runnin...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/469#discussion_r169435199 --- Diff: build.xml --- @@ -1861,4 +1861,18 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- nit: I think most of our other ant targets use "-" instead of "_" between words. ---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168886064
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +923,103 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testFailedTxnAsPartOfQuorumLoss() throws Exception {
+// 1. start up server and wait for leader election to finish
+ClientBase.setupTestEnv();
+final int SERVER_COUNT = 3;
+servers = LaunchServers(SERVER_COUNT);
+
+waitForAll(servers, States.CONNECTED);
+
+// we need to shutdown and start back up to make sure that the
create session isn't the first transaction since
+// that is rather innocuous.
+servers.shutDownAllServers();
+waitForAll(servers, States.CONNECTING);
+servers.restartAllServersAndClients(this);
+waitForAll(servers, States.CONNECTED);
+
+// 2. kill all followers
+int leader = servers.findLeader();
+Map<Long, Proposal> outstanding =
servers.mt[leader].main.quorumPeer.leader.outstandingProposals;
+// increase the tick time to delay the leader going to looking
+servers.mt[leader].main.quorumPeer.tickTime = 1;
+LOG.warn("LEADER {}", leader);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+servers.mt[i].shutdown();
+}
+}
+
+// 3. start up the followers to form a new quorum
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+servers.mt[i].start();
+}
+}
+
+// 4. wait one of the follower to be the new leader
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+// Recreate a client session since the previous session
was not persisted.
+servers.restartClient(i, this);
+waitForOne(servers.zk[i], States.CONNECTED);
+}
+}
+
+// 5. send a create request to old leader and make sure it's
synced to disk,
+//which means it acked from itself
+try {
+servers.zk[leader].create("/zk" + leader, "zk".getBytes(),
Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+Assert.fail("create /zk" + leader + " should have failed");
+} catch (KeeperException e) {
+}
+
+// just make sure that we actually did get it in process at the
+// leader
+Assert.assertEquals(1, outstanding.size());
+Proposal p = outstanding.values().iterator().next();
+Assert.assertEquals(OpCode.create, p.request.getHdr().getType());
+
+// make sure it has a chance to write it to disk
+int sleepTime = 0;
+Long longLeader = new Long(leader);
+while (!p.qvAcksetPairs.get(0).getAckset().contains(longLeader)) {
+if (sleepTime > 2000) {
+Assert.fail("Transaction not synced to disk within 1
second " + p.qvAcksetPairs.get(0).getAckset()
++ " expected " + leader);
+}
+Thread.sleep(100);
+sleepTime += 100;
+}
+
+// 6. wait for the leader to quit due to not enough followers and
come back up as a part of the new quorum
+sleepTime = 0;
+Follower f = servers.mt[leader].main.quorumPeer.follower;
+while (f == null || !f.isRunning()) {
+if (sleepTime > 10_000) {
--- End diff --
nitpick: can we reuse the ticktime here to make the relationship more
obvious?
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168887935
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +923,103 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testFailedTxnAsPartOfQuorumLoss() throws Exception {
+// 1. start up server and wait for leader election to finish
+ClientBase.setupTestEnv();
+final int SERVER_COUNT = 3;
+servers = LaunchServers(SERVER_COUNT);
+
+waitForAll(servers, States.CONNECTED);
+
+// we need to shutdown and start back up to make sure that the
create session isn't the first transaction since
+// that is rather innocuous.
+servers.shutDownAllServers();
+waitForAll(servers, States.CONNECTING);
+servers.restartAllServersAndClients(this);
+waitForAll(servers, States.CONNECTED);
+
+// 2. kill all followers
+int leader = servers.findLeader();
+Map<Long, Proposal> outstanding =
servers.mt[leader].main.quorumPeer.leader.outstandingProposals;
+// increase the tick time to delay the leader going to looking
+servers.mt[leader].main.quorumPeer.tickTime = 1;
+LOG.warn("LEADER {}", leader);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+servers.mt[i].shutdown();
+}
+}
+
+// 3. start up the followers to form a new quorum
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+servers.mt[i].start();
+}
+}
+
+// 4. wait one of the follower to be the new leader
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+// Recreate a client session since the previous session
was not persisted.
+servers.restartClient(i, this);
+waitForOne(servers.zk[i], States.CONNECTED);
+}
+}
+
+// 5. send a create request to old leader and make sure it's
synced to disk,
+//which means it acked from itself
+try {
+servers.zk[leader].create("/zk" + leader, "zk".getBytes(),
Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+Assert.fail("create /zk" + leader + " should have failed");
+} catch (KeeperException e) {
+}
+
+// just make sure that we actually did get it in process at the
+// leader
+Assert.assertEquals(1, outstanding.size());
+Proposal p = outstanding.values().iterator().next();
+Assert.assertEquals(OpCode.create, p.request.getHdr().getType());
+
+// make sure it has a chance to write it to disk
+int sleepTime = 0;
+Long longLeader = new Long(leader);
+while (!p.qvAcksetPairs.get(0).getAckset().contains(longLeader)) {
+if (sleepTime > 2000) {
+Assert.fail("Transaction not synced to disk within 1
second " + p.qvAcksetPairs.get(0).getAckset()
++ " expected " + leader);
+}
+Thread.sleep(100);
+sleepTime += 100;
+}
+
+// 6. wait for the leader to quit due to not enough followers and
come back up as a part of the new quorum
+sleepTime = 0;
+Follower f = servers.mt[leader].main.quorumPeer.follower;
+while (f == null || !f.isRunning()) {
+if (sleepTime > 10_000) {
+Assert.fail("Took too long for old leader to time out " +
servers.mt[leader].main.quorumPeer.getPeerState());
+}
+Thread.sleep(100);
+sleepTime += 100;
+f = servers.mt[leader].main.quorumPeer.follower;
+}
+servers.mt[leader].shutdown();
--- End diff --
why do we need this?
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168884819
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -465,6 +470,37 @@ private void waitForAll(ZooKeeper[] zks, States state)
throws InterruptedExcepti
private static class Servers {
MainThread mt[];
ZooKeeper zk[];
+int[] clientPorts;
+
+public void shutDownAllServers() throws InterruptedException {
+for (MainThread t: mt) {
+t.shutdown();
+}
+}
+
+public void restartAllServersAndClients(Watcher watcher) throws
IOException {
+for (MainThread t : mt) {
+if (!t.isAlive()) {
+t.start();
+}
+}
+for (int i = 0; i < zk.length; i++) {
+restartClient(i, watcher);
+}
+}
+
+public void restartClient(int i, Watcher watcher) throws
IOException {
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, watcher);
+}
+
+public int findLeader() {
--- End diff --
there are other places in this test class that benefit from this
refactoring. Would you mind cleaning that up?
---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r168883423 --- Diff: build.xml --- @@ -1861,4 +1876,18 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- As we discussed offline please move these useful targets to a new JIRA. ---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168857757
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +888,127 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testTxnAheadSnapInRetainDB() throws Exception {
+// 1. start up server and wait for leader election to finish
+ClientBase.setupTestEnv();
+final int SERVER_COUNT = 3;
+final int clientPorts[] = new int[SERVER_COUNT];
+StringBuilder sb = new StringBuilder();
+for (int i = 0; i < SERVER_COUNT; i++) {
+clientPorts[i] = PortAssignment.unique();
+sb.append("server." + i + "=127.0.0.1:" +
PortAssignment.unique() + ":" + PortAssignment.unique() + ";" + clientPorts[i]
+ "\n");
+}
+String quorumCfgSection = sb.toString();
+
+MainThread mt[] = new MainThread[SERVER_COUNT];
+ZooKeeper zk[] = new ZooKeeper[SERVER_COUNT];
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i] = new MainThread(i, clientPorts[i], quorumCfgSection);
+mt[i].start();
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// we need to shutdown and start back up to make sure that the
create session isn't the first transaction since
+// that is rather innocuous.
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].shutdown();
+}
+
+waitForAll(zk, States.CONNECTING);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].start();
+// Recreate a client session since the previous session was
not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// 2. kill all followers
+int leader = -1;
+Map<Long, Proposal> outstanding = null;
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (mt[i].main.quorumPeer.leader != null) {
+leader = i;
+outstanding =
mt[leader].main.quorumPeer.leader.outstandingProposals;
+// increase the tick time to delay the leader going to
looking
+mt[leader].main.quorumPeer.tickTime = 1;
+}
+}
+LOG.warn("LEADER {}", leader);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].shutdown();
+}
+}
+
+// 3. start up the followers to form a new quorum
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].start();
+}
+}
+
+// 4. wait one of the follower to be the leader
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+// Recreate a client session since the previous session
was not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+waitForOne(zk[i], States.CONNECTED);
+}
+}
+
+// 5. send a create request to leader and make sure it's synced to
disk,
+//which means it acked from itself
+try {
+zk[leader].create("/zk" + leader, "zk".getBytes(),
Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+Assert.fail("create /zk" + leader + " should have failed");
+} catch (KeeperException e) {
+}
+
+// just make sure that we actually did get it in process at the
+// leader
+Assert.assertTrue(outstanding.size() == 1);
+Proposal p = (Proposal) outstanding.values().iterator().next();
+Assert.assertTrue(p.request.getHdr().getType() == OpCode.create);
+
+// make sure it has a chance to write it to disk
+Thread.sleep(1000);
--- End diff --
@revans2 take a look at `testElectionFraud`, specifically:
https://github.com/apache/zookeeper/blob/master/src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java#L383
and
https://github.com/apache/zookeeper/blob/maste
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168857052
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -435,7 +435,7 @@ private void waitForOne(ZooKeeper zk, States state)
throws InterruptedException
int iterations = ClientBase.CONNECTION_TIMEOUT / 500;
while (zk.getState() != state) {
if (iterations-- == 0) {
-throw new RuntimeException("Waiting too long");
+throw new RuntimeException("Waiting too long " +
zk.getState() + " != " + state);
--- End diff --
Since @anmolnar thinks it is valuable, I think it is fine for it to be left
in.
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168651275
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +888,127 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testTxnAheadSnapInRetainDB() throws Exception {
+// 1. start up server and wait for leader election to finish
+ClientBase.setupTestEnv();
+final int SERVER_COUNT = 3;
+final int clientPorts[] = new int[SERVER_COUNT];
+StringBuilder sb = new StringBuilder();
+for (int i = 0; i < SERVER_COUNT; i++) {
+clientPorts[i] = PortAssignment.unique();
+sb.append("server." + i + "=127.0.0.1:" +
PortAssignment.unique() + ":" + PortAssignment.unique() + ";" + clientPorts[i]
+ "\n");
+}
+String quorumCfgSection = sb.toString();
+
+MainThread mt[] = new MainThread[SERVER_COUNT];
+ZooKeeper zk[] = new ZooKeeper[SERVER_COUNT];
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i] = new MainThread(i, clientPorts[i], quorumCfgSection);
+mt[i].start();
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// we need to shutdown and start back up to make sure that the
create session isn't the first transaction since
+// that is rather innocuous.
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].shutdown();
+}
+
+waitForAll(zk, States.CONNECTING);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].start();
+// Recreate a client session since the previous session was
not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// 2. kill all followers
+int leader = -1;
+Map<Long, Proposal> outstanding = null;
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (mt[i].main.quorumPeer.leader != null) {
+leader = i;
+outstanding =
mt[leader].main.quorumPeer.leader.outstandingProposals;
+// increase the tick time to delay the leader going to
looking
+mt[leader].main.quorumPeer.tickTime = 1;
+}
+}
+LOG.warn("LEADER {}", leader);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].shutdown();
+}
+}
+
+// 3. start up the followers to form a new quorum
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].start();
+}
+}
+
+// 4. wait one of the follower to be the leader
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+// Recreate a client session since the previous session
was not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+waitForOne(zk[i], States.CONNECTED);
+}
+}
+
+// 5. send a create request to leader and make sure it's synced to
disk,
+//which means it acked from itself
+try {
+zk[leader].create("/zk" + leader, "zk".getBytes(),
Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+Assert.fail("create /zk" + leader + " should have failed");
+} catch (KeeperException e) {
+}
+
+// just make sure that we actually did get it in process at the
+// leader
+Assert.assertTrue(outstanding.size() == 1);
+Proposal p = (Proposal) outstanding.values().iterator().next();
--- End diff --
Do we need this cast?
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168649459
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +888,127 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testTxnAheadSnapInRetainDB() throws Exception {
--- End diff --
nit: I don't think we use the terminology "RetainDB" anywhere else. Perhaps
we can get rid of "retain"?
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168649906
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +888,127 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testTxnAheadSnapInRetainDB() throws Exception {
+// 1. start up server and wait for leader election to finish
+ClientBase.setupTestEnv();
+final int SERVER_COUNT = 3;
+final int clientPorts[] = new int[SERVER_COUNT];
+StringBuilder sb = new StringBuilder();
+for (int i = 0; i < SERVER_COUNT; i++) {
+clientPorts[i] = PortAssignment.unique();
+sb.append("server." + i + "=127.0.0.1:" +
PortAssignment.unique() + ":" + PortAssignment.unique() + ";" + clientPorts[i]
+ "\n");
+}
+String quorumCfgSection = sb.toString();
+
+MainThread mt[] = new MainThread[SERVER_COUNT];
+ZooKeeper zk[] = new ZooKeeper[SERVER_COUNT];
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i] = new MainThread(i, clientPorts[i], quorumCfgSection);
--- End diff --
is there any reason we can't use the existing test infra to clean this up a
little
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168649723
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +888,127 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testTxnAheadSnapInRetainDB() throws Exception {
+// 1. start up server and wait for leader election to finish
+ClientBase.setupTestEnv();
+final int SERVER_COUNT = 3;
+final int clientPorts[] = new int[SERVER_COUNT];
+StringBuilder sb = new StringBuilder();
+for (int i = 0; i < SERVER_COUNT; i++) {
+clientPorts[i] = PortAssignment.unique();
+sb.append("server." + i + "=127.0.0.1:" +
PortAssignment.unique() + ":" + PortAssignment.unique() + ";" + clientPorts[i]
+ "\n");
+}
+String quorumCfgSection = sb.toString();
+
+MainThread mt[] = new MainThread[SERVER_COUNT];
+ZooKeeper zk[] = new ZooKeeper[SERVER_COUNT];
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i] = new MainThread(i, clientPorts[i], quorumCfgSection);
+mt[i].start();
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// we need to shutdown and start back up to make sure that the
create session isn't the first transaction since
+// that is rather innocuous.
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].shutdown();
+}
+
+waitForAll(zk, States.CONNECTING);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].start();
+// Recreate a client session since the previous session was
not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// 2. kill all followers
+int leader = -1;
+Map<Long, Proposal> outstanding = null;
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (mt[i].main.quorumPeer.leader != null) {
+leader = i;
+outstanding =
mt[leader].main.quorumPeer.leader.outstandingProposals;
+// increase the tick time to delay the leader going to
looking
+mt[leader].main.quorumPeer.tickTime = 1;
+}
+}
+LOG.warn("LEADER {}", leader);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].shutdown();
+}
+}
+
+// 3. start up the followers to form a new quorum
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].start();
+}
+}
+
+// 4. wait one of the follower to be the leader
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+// Recreate a client session since the previous session
was not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+waitForOne(zk[i], States.CONNECTED);
+}
+}
+
+// 5. send a create request to leader and make sure it's synced to
disk,
+//which means it acked from itself
+try {
+zk[leader].create("/zk" + leader, "zk".getBytes(),
Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+Assert.fail("create /zk" + leader + " should have failed");
+} catch (KeeperException e) {
+}
+
+// just make sure that we actually did get it in process at the
+// leader
+Assert.assertTrue(outstanding.size() == 1);
+Proposal p = (Proposal) outstanding.values().iterator().next();
+Assert.assertTrue(p.request.getHdr().getType() == OpCode.create);
+
+// make sure it has a chance to write it to disk
+Thread.sleep(1000);
+p.qvAcksetPairs.get(0).getAckset().contains(leader);
+
+// 6. wait the leader to quit due to no enough followers
+Thread.sleep(4000);
+//waitForOne(zk[leader], States.CONNECTING);
--- End diff --
remove this
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168649080
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -435,7 +435,7 @@ private void waitForOne(ZooKeeper zk, States state)
throws InterruptedException
int iterations = ClientBase.CONNECTION_TIMEOUT / 500;
while (zk.getState() != state) {
if (iterations-- == 0) {
-throw new RuntimeException("Waiting too long");
+throw new RuntimeException("Waiting too long " +
zk.getState() + " != " + state);
--- End diff --
nit: let's minimize unrelated test changes and whitespace changes
---
[GitHub] zookeeper pull request #453: ZOOKEEPER-2845: Apply commit log when restartin...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/453#discussion_r168653437
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -888,4 +888,127 @@ public void testWithOnlyMinSessionTimeout() throws
Exception {
maxSessionTimeOut, quorumPeer.getMaxSessionTimeout());
}
+@Test
+public void testTxnAheadSnapInRetainDB() throws Exception {
+// 1. start up server and wait for leader election to finish
+ClientBase.setupTestEnv();
+final int SERVER_COUNT = 3;
+final int clientPorts[] = new int[SERVER_COUNT];
+StringBuilder sb = new StringBuilder();
+for (int i = 0; i < SERVER_COUNT; i++) {
+clientPorts[i] = PortAssignment.unique();
+sb.append("server." + i + "=127.0.0.1:" +
PortAssignment.unique() + ":" + PortAssignment.unique() + ";" + clientPorts[i]
+ "\n");
+}
+String quorumCfgSection = sb.toString();
+
+MainThread mt[] = new MainThread[SERVER_COUNT];
+ZooKeeper zk[] = new ZooKeeper[SERVER_COUNT];
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i] = new MainThread(i, clientPorts[i], quorumCfgSection);
+mt[i].start();
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// we need to shutdown and start back up to make sure that the
create session isn't the first transaction since
+// that is rather innocuous.
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].shutdown();
+}
+
+waitForAll(zk, States.CONNECTING);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+mt[i].start();
+// Recreate a client session since the previous session was
not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+}
+
+waitForAll(zk, States.CONNECTED);
+
+// 2. kill all followers
+int leader = -1;
+Map<Long, Proposal> outstanding = null;
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (mt[i].main.quorumPeer.leader != null) {
+leader = i;
+outstanding =
mt[leader].main.quorumPeer.leader.outstandingProposals;
+// increase the tick time to delay the leader going to
looking
+mt[leader].main.quorumPeer.tickTime = 1;
+}
+}
+LOG.warn("LEADER {}", leader);
+
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].shutdown();
+}
+}
+
+// 3. start up the followers to form a new quorum
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+mt[i].start();
+}
+}
+
+// 4. wait one of the follower to be the leader
+for (int i = 0; i < SERVER_COUNT; i++) {
+if (i != leader) {
+// Recreate a client session since the previous session
was not persisted.
+zk[i] = new ZooKeeper("127.0.0.1:" + clientPorts[i],
ClientBase.CONNECTION_TIMEOUT, this);
+waitForOne(zk[i], States.CONNECTED);
+}
+}
+
+// 5. send a create request to leader and make sure it's synced to
disk,
+//which means it acked from itself
+try {
+zk[leader].create("/zk" + leader, "zk".getBytes(),
Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+Assert.fail("create /zk" + leader + " should have failed");
+} catch (KeeperException e) {
+}
+
+// just make sure that we actually did get it in process at the
+// leader
+Assert.assertTrue(outstanding.size() == 1);
+Proposal p = (Proposal) outstanding.values().iterator().next();
+Assert.assertTrue(p.request.getHdr().getType() == OpCode.create);
+
+// make sure it has a chance to write it to disk
+Thread.sleep(1000);
--- End diff --
There is a lot of `Thread.sleep()` going on and I would like to find a way
to minimize that. Apache infra can occasionally be quite slow (it can starve
threads) and tests with many `Thread.sleep()`s in them have historically been
quite flaky.
[GitHub] zookeeper issue #461: ZOOKEEPER-2978: missing list
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/461 @achimbab Thanks for the patch. I'm wondering if you ran into this bug while running ZooKeeper? This code is pretty old and should never actually be executed (for reasons described here: https://github.com/apache/zookeeper/pull/461/files#diff-e08c1cfd3802a0c3156847175ab8e24bR514). I would be happy to merge in this fix but please rename the JIRA and the pull request to something more descriptive (as I do not have permissions to change the name of a Pull Request). Something like "fix potential null pointer exception when deleting node". ---
[GitHub] zookeeper issue #462: ZOOKEEPER-2980 Backport ZOOKEEPER-2939 Deal with maxbu...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/462 The patch looks good. I'm just wondering why we need this in 3.4. As I believe we generally try not to add features to that branch at this point that don't improve security or stability. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168575513
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
I guess my real question is, do we need any of the changes in this file?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168578617
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possi
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168568596
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -47,59 +51,169 @@
private int currentIndex = -1;
+// Don't re-resolve on first next() call
+private boolean connectedSinceNext = true;
+
+private Resolver resolver;
+
/**
* Constructs a SimpleHostSet.
- *
+ *
* @param serverAddresses
*possibly unresolved ZooKeeper server addresses
* @throws IllegalArgumentException
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection
serverAddresses) {
+this.resolver = new Resolver() {
+@Override
+public InetAddress[] getAllByName(String name) throws
UnknownHostException {
+return InetAddress.getAllByName(name);
+}
+};
+init(serverAddresses);
+}
+
+/**
+ * Introduced for testing purposes. getAllByName() is a static method
of InetAddress, therefore cannot be easily mocked.
+ * By abstraction of Resolver interface we can easily inject a mocked
implementation in tests.
+ *
+ * @param serverAddresses
+ *possibly unresolved ZooKeeper server addresses
+ * @param resolver
+ *custom resolver implementation
+ * @throws IllegalArgumentException
+ * if serverAddresses is empty or resolves to an empty list
+ */
+public StaticHostProvider(Collection
serverAddresses, Resolver resolver) {
+this.resolver = resolver;
+init(serverAddresses);
+}
+
+/**
+ * Common init method for all constructors.
+ * Resolve all unresolved server addresses, put them in a list and
shuffle.
+ */
+private void init(Collection serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
this.resolver.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
+
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private Strin
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168572502
--- Diff: src/java/test/org/apache/zookeeper/test/ClientPortBindTest.java
---
@@ -104,7 +104,7 @@ public void testBindByAddress() throws Exception {
try {
startSignal.await(CONNECTION_TIMEOUT,
TimeUnit.MILLISECONDS);
-Assert.assertTrue("count == 0", startSignal.getCount() == 0);
+Assert.assertTrue("count == " + startSignal.getCount(),
startSignal.getCount() == 0);
--- End diff --
I'm not a huge fan of calling `getCount` more than once, since I think the
value could change between invocations. Why not just use the return value from
`await` in the line above?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168576090
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -47,59 +51,169 @@
private int currentIndex = -1;
+// Don't re-resolve on first next() call
+private boolean connectedSinceNext = true;
+
+private Resolver resolver;
+
/**
* Constructs a SimpleHostSet.
- *
+ *
* @param serverAddresses
*possibly unresolved ZooKeeper server addresses
* @throws IllegalArgumentException
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection
serverAddresses) {
+this.resolver = new Resolver() {
+@Override
+public InetAddress[] getAllByName(String name) throws
UnknownHostException {
+return InetAddress.getAllByName(name);
+}
+};
+init(serverAddresses);
+}
+
+/**
+ * Introduced for testing purposes. getAllByName() is a static method
of InetAddress, therefore cannot be easily mocked.
+ * By abstraction of Resolver interface we can easily inject a mocked
implementation in tests.
+ *
+ * @param serverAddresses
+ *possibly unresolved ZooKeeper server addresses
+ * @param resolver
+ *custom resolver implementation
+ * @throws IllegalArgumentException
+ * if serverAddresses is empty or resolves to an empty list
+ */
+public StaticHostProvider(Collection
serverAddresses, Resolver resolver) {
+this.resolver = resolver;
+init(serverAddresses);
+}
+
+/**
+ * Common init method for all constructors.
+ * Resolve all unresolved server addresses, put them in a list and
shuffle.
+ */
+private void init(Collection serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
this.resolver.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
+
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private Strin
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168576884
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -119,6 +120,68 @@ public void testTwoInvalidHostAddresses() {
new StaticHostProvider(list);
}
+@Test
+public void testReResolvingSingle() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to a single address
+list.clear();
--- End diff --
do we need this?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r168565039 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -18,6 +18,10 @@ package org.apache.zookeeper.client; +import org.apache.yetus.audience.InterfaceAudience; --- End diff -- nit: move this back ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r168564981 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -18,6 +18,10 @@ package org.apache.zookeeper.client; +import org.apache.yetus.audience.InterfaceAudience; --- End diff -- nit: move this back ---
[GitHub] zookeeper pull request #459: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/459#discussion_r168301575
--- Diff: src/java/test/org/apache/zookeeper/test/ClientBase.java ---
@@ -351,20 +351,37 @@ static void verifyThreadTerminated(Thread thread,
long millis)
}
}
+public static File createEmptyTestDir() throws IOException {
+return createTmpDir(BASETEST, false);
+}
public static File createTmpDir() throws IOException {
-return createTmpDir(BASETEST);
+return createTmpDir(BASETEST, true);
}
-static File createTmpDir(File parentDir) throws IOException {
+
+static File createTmpDir(File parentDir, boolean createInitFile)
throws IOException {
File tmpFile = File.createTempFile("test", ".junit", parentDir);
// don't delete tmpFile - this ensures we don't attempt to create
// a tmpDir with a duplicate name
File tmpDir = new File(tmpFile + ".dir");
Assert.assertFalse(tmpDir.exists()); // never true if tmpfile does
it's job
Assert.assertTrue(tmpDir.mkdirs());
+// todo not every tmp directory needs this file
--- End diff --
do we need this todo?
---
[GitHub] zookeeper pull request #459: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/459#discussion_r168301408
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileSnap.java ---
@@ -51,19 +51,22 @@
public class FileSnap implements SnapShot {
File snapDir;
private volatile boolean close = false;
-private static final int VERSION=2;
-private static final long dbId=-1;
+private static final int VERSION = 2;
--- End diff --
Please revert this
---
[GitHub] zookeeper pull request #458: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/458#discussion_r168301155
--- Diff: src/java/test/org/apache/zookeeper/test/ClientBase.java ---
@@ -368,22 +368,37 @@ static void verifyThreadTerminated(Thread thread,
long millis)
}
}
+public static File createEmptyTestDir() throws IOException {
+return createTmpDir(BASETEST, false);
+}
public static File createTmpDir() throws IOException {
-return createTmpDir(BASETEST);
+return createTmpDir(BASETEST, true);
}
-static File createTmpDir(File parentDir) throws IOException {
+static File createTmpDir(File parentDir, boolean createInitFile)
throws IOException {
File tmpFile = File.createTempFile("test", ".junit", parentDir);
// don't delete tmpFile - this ensures we don't attempt to create
// a tmpDir with a duplicate name
File tmpDir = new File(tmpFile + ".dir");
Assert.assertFalse(tmpDir.exists()); // never true if tmpfile does
it's job
Assert.assertTrue(tmpDir.mkdirs());
+// todo not every tmp directory needs this file
--- End diff --
why is this todo here?
---
[GitHub] zookeeper issue #463: ZOOKEEPER-2981 Fix build on branch-3.5
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/463 Thanks @anmolnar This has been merged. For whatever reason the merge script decided not to close this PR so please do that. ---
[GitHub] zookeeper issue #463: ZOOKEEPER-2939 Deal with maxbuffer as it relates to pr...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/463 @anmolnar Thank you for fixing this so quickly. Please create a new JIRA for this (as things get complicated when we have many commits for the same JIRA ticket) and I will merge this. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r167013851
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possi
[GitHub] zookeeper issue #440: ZOOKEEPER-2939 Deal with maxbuffer as it relates to pr...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/440 I merged https://github.com/apache/zookeeper/pull/415. Please go ahead and create a new JIRA to correspond to this PR and I'll mark ZOOKEEPER-2939 as resolved. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166479687
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -117,8 +116,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
+list.add(InetSocketAddress.createUnresolved("www.apache.org",
1234));
--- End diff --
Yeah, this is annoying. Although, another possibility would be to put the
dns calls in a method and then subclass `StaticHostProvider` for the tests and
overriding this method to return what you want.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166476703
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possi
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166476047
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
not going to push too hard on this, but I think you can just use log4j
everywhere like in `QuorumPeerMainTest`
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r166097161 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -6,9 +6,9 @@ * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * --- End diff -- was this accidental? ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166103404
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possi
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166105622
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -117,8 +116,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
+list.add(InetSocketAddress.createUnresolved("www.apache.org",
1234));
--- End diff --
I'm wondering if it's possible to mock this out? It would be great if our
unit tests were not dependent on some other infrastructure.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166102194
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possi
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165525652
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
why is this necessary?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165527366
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -57,29 +62,12 @@
*/
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
-try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
-for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
-}
-} catch (UnknownHostException e) {
+ try {
--- End diff --
something is wrong with the indentation here
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r165524377 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -25,6 +25,8 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.lang.reflect.InvocationTargetException; --- End diff -- i think these imports are unused, and there are some others elsewhere in the code ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165529085
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -91,15 +79,106 @@ public
StaticHostProvider(Collection serverAddresses) {
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+int getNextAdded() {
+return nextAdded;
+}
+
+int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
--- End diff --
would you mind explaining exactly under which conditions we reresolve the
hostname and under which conditions we try the next one in the host list? My
reading is that this reresolves everything if the client fails to connect to
two hosts in a row. Is this the desired behavior?
And do we always reresolve all serverAddresses?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r165525113 --- Diff: src/java/test/org/apache/zookeeper/client/StaticHostProviderTest.java --- @@ -16,7 +16,7 @@ * limitations under the License. */ -package org.apache.zookeeper.test; +package org.apache.zookeeper.client; --- End diff -- this doesn't look right ---
[GitHub] zookeeper issue #415: ZOOKEEPER-2939: Added last/min/max proposal size JMX b...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/415 @phunt I think this is ready to merge. Just waiting on your +1. ---
[GitHub] zookeeper issue #296: ZOOKEEPER-2824: `FileChannel#size` info should be adde...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/296 Merged. Thanks @asdf2014! ---
[GitHub] zookeeper pull request #296: ZOOKEEPER-2824: `FileChannel#size` info should ...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/296#discussion_r164878614
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java ---
@@ -332,11 +333,13 @@ public synchronized void commit() throws IOException {
if (forceSync) {
long startSyncNS = System.nanoTime();
-log.getChannel().force(false);
+FileChannel channel = log.getChannel();
+channel.force(false);
syncElapsedMS =
TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startSyncNS);
if (syncElapsedMS > fsyncWarningThresholdMS) {
-LOG.warn("fsync-ing the write ahead log in "
+LOG.warn("fsync-ing the write ahead log ("
++ channel.size() + " bytes) in "
--- End diff --
@asdf2014 as soon as @phunt's comment is addressed I can merge this in.
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/450#discussion_r163395510 --- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java --- @@ -50,6 +50,9 @@ private static final String SNAP_DIR="snapDir"; private static final String LOG_DIR="logDir"; private static final String DB_FORMAT_CONV="dbFormatConversion"; + +private static final String LOG_FILE_PREFIX = "log"; --- End diff -- Can we use these field when actually creating the log and snapshot files? ---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r163395848
--- Diff:
src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java ---
@@ -136,13 +136,51 @@ public FileTxnSnapLog(File dataDir, File snapDir)
throws IOException {
throw new DatadirException("Cannot write to snap directory " +
this.snapDir);
}
+// check content of transaction log and snapshot dirs if they are
two different directories
--- End diff --
Could we add a reference to the discussion that explains why this whole
check is needed in the comments and/or the exception that the user sees?
---
[GitHub] zookeeper pull request #450: ZOOKEEPER-2967: Add check to validate dataDir a...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/450#discussion_r163472195
--- Diff:
src/java/test/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
---
@@ -159,4 +159,222 @@ public void onTxnLoaded(TxnHeader hdr, Record rec) {
}
}
}
+
+@Test
--- End diff --
there is a lot of code duplication in these tests, i'm wondering if they
can be cleaned up.
---
[GitHub] zookeeper pull request #446: ZOOKEEPER-1580:QuorumPeer.setRunning is not use...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/446#discussion_r163392345
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
@@ -1751,7 +1751,7 @@ public synchronized void initConfigInZKDatabase() {
if (zkDb != null) zkDb.initConfigInZKDatabase(getQuorumVerifier());
}
-public void setRunning(boolean running) {
+private void setRunning(boolean running) {
--- End diff --
+1 to @anmolnar i think we can remote the setter
---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/440#discussion_r163390292
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --
From:
http://metrics.dropwizard.io/3.1.0/manual/core/#exponentially-decaying-reservoirs
> A histogram with an exponentially decaying reservoir produces quantiles
which are representative of (roughly) the last five minutes of data.
My guess is that we would want more than data representing the last 5
minutes right?
---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r163373344 --- Diff: build.xml --- @@ -1406,50 +1410,53 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + + + + + + + - + --- End diff -- we should update this description to show that we have an ant task that can be used instead of specifying a system property ---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r163377320 --- Diff: build.xml --- @@ -124,6 +160,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- My concern here is around classpath issues. Ideally I would like to make sure that clover and its dependencies (my understanding is that there are none currently but this could change) are only included when we are instrumenting coverage. The given setup may see us including clover in the test classpath even when not intended. For example, a developer wants to run the tests with clover and then immediately after without. If my understanding is correct, it there is no `clean` before those two executions clover will be included in the classpath of the second. ---
[GitHub] zookeeper pull request #443: ZOOKEEPER-2955: Enable Clover code coverage rep...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/443#discussion_r163372311 --- Diff: build.xml --- @@ -23,6 +23,48 @@ xmlns:artifact="antlib:org.apache.maven.artifact.ant" xmlns:maven="antlib:org.apache.maven.artifact.ant" xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> + --- End diff -- why was this stuff moved? ---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/440#discussion_r160889741 --- Diff: build.xml --- @@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> - + --- End diff -- Can we just exclude org.slf4j#slf4j-api;1.7.22 ? ---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/440#discussion_r160483835 --- Diff: build.xml --- @@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> - + --- End diff -- So do we still need this lower version? ---
[GitHub] zookeeper issue #437: ZOOKEEPER-2961: Fix testElectionFraud Flakyness
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/437 Hi @anmolnar - I had a description in the JIRA that I just copied here. I hope it clears things up. ---
[GitHub] zookeeper pull request #438: ZOOKEEPER-2959: ignore accepted epoch and LEADE...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/438#discussion_r160286100
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -1183,8 +1183,10 @@ public long getEpochToPropose(long sid, long
lastAcceptedEpoch) throws Interrupt
if (lastAcceptedEpoch >= epoch) {
epoch = lastAcceptedEpoch+1;
}
-connectingFollowers.add(sid);
QuorumVerifier verifier = self.getQuorumVerifier();
+if(verifier.getVotingMembers().containsKey(sid)) {
--- End diff --
I'm wondering if this logic is best suited for the `QuorumVerifier`. In
other words, the quorum verifier should be able to determine if a quorum is
present from a set of ids while taking into account which sids represent voting
members.
---
[GitHub] zookeeper issue #439: ZOOKEEPER-1621: Delete and skip txn log with incomplet...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/439 @abhishekrai Looking through the JIRA I found: > This has been a recurring problem for us in production because our app's operating environment occasionally causes a Zookeeper server's disk to become full. After that, the server invariably runs into this problem - perhaps because there's something else that deterministically triggers a log rotation when the previous txn log throws an IOException due to disk full? Do we have evidence that the log roll is being triggered "deterministically"? It would be great to know for sure that we are handling the disk filling up appropriately all the time rather than just a work around for special cases. ---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/440#discussion_r160233203 --- Diff: ivy.xml --- @@ -133,6 +133,12 @@ + + +
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/440#discussion_r159986909 --- Diff: src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java --- @@ -0,0 +1,70 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.quorum; + +import com.codahale.metrics.ExponentiallyDecayingReservoir; --- End diff -- I don't see much being pulled in from dropwizard, only codahale. Is codahale.metrics available independently? It doesn't look like it but I only checked briefly. ---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/440#discussion_r160236436
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/LeaderBeanTest.java ---
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.jute.OutputArchive;
+import org.apache.jute.Record;
+import org.apache.zookeeper.server.Request;
+import org.apache.zookeeper.server.ZKDatabase;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.apache.zookeeper.server.quorum.flexible.QuorumVerifier;
+import org.apache.zookeeper.server.util.SerializeUtils;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.txn.TxnHeader;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+import java.io.File;
+import java.io.IOException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+
+public class LeaderBeanTest {
+private Leader leader;
+private LeaderBean leaderBean;
+private FileTxnSnapLog fileTxnSnapLog;
+private LeaderZooKeeperServer zks;
+private QuorumPeer qp;
+
+@Before
+public void setUp() throws IOException {
+qp = new QuorumPeer();
+QuorumVerifier quorumVerifierMock = mock(QuorumVerifier.class);
+qp.setQuorumVerifier(quorumVerifierMock, false);
+File tmpDir = ClientBase.createEmptyTestDir();
+fileTxnSnapLog = new FileTxnSnapLog(new File(tmpDir, "data"),
+new File(tmpDir, "data_txnlog"));
+ZKDatabase zkDb = new ZKDatabase(fileTxnSnapLog);
+
+zks = new LeaderZooKeeperServer(fileTxnSnapLog, qp, zkDb);
+leader = new Leader(qp, zks);
+leaderBean = new LeaderBean(leader, zks);
+}
+
+@After
+public void tearDown() throws IOException {
+fileTxnSnapLog.close();
+}
+
+@Test
+public void testGetName() {
+assertEquals("Leader", leaderBean.getName());
+}
+
+@Test
+public void testGetCurrentZxid() {
+// Arrange
+zks.setZxid(1);
+
+// Assert
+assertEquals("0x1", leaderBean.getCurrentZxid());
+}
+
+@Test
+public void testGetElectionTimeTaken() {
+// Arrange
+qp.setElectionTimeTaken(1);
+
+// Assert
+assertEquals(1, leaderBean.getElectionTimeTaken());
+}
+
+private Request createMockRequest() throws IOException {
--- End diff --
is this ever used?
---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/440#discussion_r159987502
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --
I won't pretend to know much about exponentially decaying reservoirs. I'm
curious what the behavior is with minimum and maximum values. Are these
guaranteed to always be exact and for what time period?
---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/440#discussion_r159985176 --- Diff: build.xml --- @@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant"> - + --- End diff -- what is the reason for the downgrade? ---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/440#discussion_r160236050 --- Diff: src/java/test/org/apache/zookeeper/server/quorum/LeaderBeanTest.java --- @@ -0,0 +1,119 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.zookeeper.server.quorum; + +import org.apache.jute.OutputArchive; --- End diff -- nit: there are a couple unused imports here ---
[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/440#discussion_r160249680
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --
Perhaps a user configurable SlidingTimeWindowReservoir may be more
appropriate?
---
[GitHub] zookeeper pull request #437: ZOOKEEPER-2961: Fix testElectionFraud Flakyness
GitHub user afine opened a pull request: https://github.com/apache/zookeeper/pull/437 ZOOKEEPER-2961: Fix testElectionFraud Flakyness You can merge this pull request into a Git repository by running: $ git pull https://github.com/afine/zookeeper ZOOKEEPER-2961_master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/437.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 #437 commit f0a64245102eda07da53bf2842620bde61146a49 Author: Abraham Fine <afine@...> Date: 2017-12-22T22:12:26Z ZOOKEEPER-2961: Fix testElectionFraud Flakyness ---
[GitHub] zookeeper pull request #436: ZOOKEEPER-2249: CRC check failed when preAllocS...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/436#discussion_r158392075
--- Diff: src/java/main/org/apache/zookeeper/server/persistence/Util.java
---
@@ -211,7 +211,7 @@ public static long padLogFile(FileOutputStream f,long
currentSize,
long preAllocSize) throws IOException{
long position = f.getChannel().position();
if (position + 4096 >= currentSize) {
-currentSize = currentSize + preAllocSize;
+currentSize = position + preAllocSize;
--- End diff --
Fixed
---
[GitHub] zookeeper pull request #436: [WIP] ZOOKEEPER-2249: CRC check failed when pre...
GitHub user afine opened a pull request: https://github.com/apache/zookeeper/pull/436 [WIP] ZOOKEEPER-2249: CRC check failed when preAllocSize smaller than node data You can merge this pull request into a Git repository by running: $ git pull https://github.com/afine/zookeeper ZOOKEEPER-2249 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/436.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 #436 commit 99236e2ddb10de6d20e5afea7e79751404ae48c6 Author: Abraham Fine <af...@apache.org> Date: 2017-12-19T02:33:11Z ZOOKEEPER-2249: CRC check failed when preAllocSize smaller than node data ---
[GitHub] zookeeper pull request #433: ZOOKEEPER-2953: Flaky Test: testNoLogBeforeLead...
Github user afine closed the pull request at: https://github.com/apache/zookeeper/pull/433 ---
[GitHub] zookeeper pull request #434: ZOOKEEPER-2953: Flaky Test: testNoLogBeforeLead...
Github user afine closed the pull request at: https://github.com/apache/zookeeper/pull/434 ---
[GitHub] zookeeper pull request #433: ZOOKEEPER-2953: Flaky Test: testNoLogBeforeLead...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/433#discussion_r157004899
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -335,6 +336,100 @@ public void testHighestZxidJoinLate() throws
Exception {
output[0], 2);
}
+/**
+ * This test validates that if a quorum member determines that it is
leader without the support of the rest of the
+ * quorum (the other members do not believe it to be the leader) it
will stop attempting to lead and become a follower.
+ *
+ * @throws IOException
+ * @throws InterruptedException
+ */
+@Test
+public void testElectionFraud() throws IOException,
InterruptedException {
+// capture QuorumPeer logging
+Layout layout =
Logger.getRootLogger().getAppender("CONSOLE").getLayout();
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.INFO);
+Logger qlogger = Logger.getLogger(QuorumPeer.class);
+qlogger.addAppender(appender);
+
+int numServers = 3;
--- End diff --
fixed
---
[GitHub] zookeeper pull request #433: ZOOKEEPER-2953: Flaky Test: testNoLogBeforeLead...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/433#discussion_r157004909
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -335,6 +336,100 @@ public void testHighestZxidJoinLate() throws
Exception {
output[0], 2);
}
+/**
+ * This test validates that if a quorum member determines that it is
leader without the support of the rest of the
+ * quorum (the other members do not believe it to be the leader) it
will stop attempting to lead and become a follower.
+ *
+ * @throws IOException
+ * @throws InterruptedException
+ */
+@Test
+public void testElectionFraud() throws IOException,
InterruptedException {
+// capture QuorumPeer logging
+Layout layout =
Logger.getRootLogger().getAppender("CONSOLE").getLayout();
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.INFO);
+Logger qlogger = Logger.getLogger(QuorumPeer.class);
+qlogger.addAppender(appender);
+
+int numServers = 3;
+
+// used for assertions later
+boolean foundLeading = false;
+boolean foundLooking = false;
+boolean foundFollowing = false;
+
+try {
+ // spin up a quorum, we use a small ticktime to make the test
run faster
+ Servers servers = LaunchServers(numServers, 500);
--- End diff --
fixed
---
[GitHub] zookeeper issue #432: ZOOKEEPER-2953: Flaky Test: testNoLogBeforeLeaderEstab...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/432 [WIP] removed and I added a description of the issue to the JIRA. ---
[GitHub] zookeeper pull request #432: ZOOKEEPER-2953: Flaky Test: testNoLogBeforeLead...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/432#discussion_r156999341
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -335,6 +336,100 @@ public void testHighestZxidJoinLate() throws
Exception {
output[0], 2);
}
+/**
+ * This test validates that if a quorum member determines that it is
leader without the support of the rest of the
+ * quorum (the other members do not believe it to be the leader) it
will stop attempting to lead and become a follower.
+ *
+ * @throws IOException
+ * @throws InterruptedException
+ */
+@Test
+public void testElectionFraud() throws IOException,
InterruptedException {
+// capture QuorumPeer logging
+Layout layout =
Logger.getRootLogger().getAppender("CONSOLE").getLayout();
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.INFO);
+Logger qlogger = Logger.getLogger(QuorumPeer.class);
+qlogger.addAppender(appender);
+
+int numServers = 3;
+
+// used for assertions later
+boolean foundLeading = false;
+boolean foundLooking = false;
+boolean foundFollowing = false;
+
+try {
+ // spin up a quorum, we use a small ticktime to make the test
run faster
+ Servers servers = LaunchServers(numServers, 500);
+
+ // find the leader
+ int trueLeader = -1;
+ for (int i = 0; i < numServers; i++) {
+if (servers.mt[i].main.quorumPeer.leader != null) {
+ trueLeader = i;
+}
+ }
+ Assert.assertTrue("There should be a leader", trueLeader >= 0);
+
+ // find a follower
+ int falseLeader = (trueLeader + 1) % numServers;
+
Assert.assertTrue(servers.mt[falseLeader].main.quorumPeer.follower != null);
--- End diff --
fixed
---
[GitHub] zookeeper issue #432: [WIP] ZOOKEEPER-2953: Flaky Test: testNoLogBeforeLeade...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/432 @anmolnar An explanation is coming, please excuse the cruft at this stage. I wanted to run the new test on apache hardware hence the [WIP] in the title. ---
[GitHub] zookeeper pull request #432: [WIP] ZOOKEEPER-2953: Flaky Test: testNoLogBefo...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/432#discussion_r156781040
--- Diff:
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java ---
@@ -335,6 +336,100 @@ public void testHighestZxidJoinLate() throws
Exception {
output[0], 2);
}
+/**
+ * This test validates that if a quorum member determines that it is
leader without the support of the rest of the
+ * quorum (the other members do not believe it to be the leader) it
will stop attempting to lead and become a follower.
+ *
+ * @throws IOException
+ * @throws InterruptedException
+ */
+@Test
+public void testElectionFraud() throws IOException,
InterruptedException {
+// capture QuorumPeer logging
+Layout layout =
Logger.getRootLogger().getAppender("CONSOLE").getLayout();
+ByteArrayOutputStream os = new ByteArrayOutputStream();
+WriterAppender appender = new WriterAppender(layout, os);
+appender.setThreshold(Level.INFO);
+Logger qlogger = Logger.getLogger(QuorumPeer.class);
+qlogger.addAppender(appender);
+
+int numServers = 3;
+
+// used for assertions later
+boolean foundLeading = false;
+boolean foundLooking = false;
+boolean foundFollowing = false;
+
+try {
+ // spin up a quorum, we use a small ticktime to make the test
run faster
+ Servers servers = LaunchServers(numServers, 500);
+
+ // find the leader
+ int trueLeader = -1;
+ for (int i = 0; i < numServers; i++) {
+if (servers.mt[i].main.quorumPeer.leader != null) {
+ trueLeader = i;
+}
+ }
+ Assert.assertTrue("There should be a leader", trueLeader >= 0);
+
+ // find a follower
+ int falseLeader = (trueLeader + 1) % numServers;
+
Assert.assertTrue(servers.mt[falseLeader].main.quorumPeer.follower != null);
+
+ // to keep the quorum peer running and force it to go into the
looking state, we kill leader election
+ // and close the connection to the leader
+ servers.mt[falseLeader].main.quorumPeer.electionAlg.shutdown();
+
servers.mt[falseLeader].main.quorumPeer.follower.getSocket().close();
+
+ // wait for the falseLeader to disconnect
+ waitForOne(servers.zk[falseLeader], States.CONNECTING);
+
+ // convince falseLeader that it is the leader
+
servers.mt[falseLeader].main.quorumPeer.setPeerState(QuorumPeer.ServerState.LEADING);
+
+ // provide time for the falseleader to realize no followers have
connected
+ // (this is twice the timeout used in Leader#getEpochToPropose)
+ Thread.sleep(2 *
servers.mt[falseLeader].main.quorumPeer.initLimit *
servers.mt[falseLeader].main.quorumPeer.tickTime);
+
+ // Restart leader election
+ servers.mt[falseLeader].main.quorumPeer.startLeaderElection();
--- End diff --
Stopping and starting leader election is necessary here to prevent a race
condition. It is possible that after the server is disconnected from the leader
it becomes a follower before the test hits
`servers.mt[falseLeader].main.quorumPeer.setPeerState(QuorumPeer.ServerState.LEADING);`
and falseLeader will never try to `lead`, defeating the purpose of the test.
---
