From c567c494d116b6893e24e5b1b812a6ff24791614 Mon Sep 17 00:00:00 2001 From: Abraham Fine Date: Mon, 17 Apr 2017 15:51:27 -0700 Subject: [PATCH 1/4] ZOOKEEPER-2759: Flaky test: org.apache.zookeeper.server.quorum.QuorumCnxManagerTest.testNoAuthLearnerConnectToAuthRequiredServerWithHigherSid --- .../apache/zookeeper/server/quorum/QuorumCnxManagerTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java index 6b6c0b4c331..05ff98939cc 100644 --- a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java +++ b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java @@ -227,6 +227,9 @@ public void testNoAuthLearnerConnectToAuthRequiredServerWithHigherSid() "QuorumLearner", true, true); peer0.connectOne(1); peer1.connectOne(0); + + peer0.halt(); + assertEventuallyNotConnected(peer0, 1); } From 112a70ba14f5cbb94e301702b328acfff54b0b02 Mon Sep 17 00:00:00 2001 From: Abraham Fine Date: Wed, 26 Apr 2017 20:56:01 -0700 Subject: [PATCH 2/4] fix tests with mockito --- ivy.xml | 2 +- .../server/quorum/QuorumCnxManager.java | 18 +++++++- .../server/quorum/QuorumCnxManagerTest.java | 46 +++++++++++++++---- 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/ivy.xml b/ivy.xml index 1a9af7612ab..8aa76c53a9d 100644 --- a/ivy.xml +++ b/ivy.xml @@ -56,7 +56,7 @@ - diff --git a/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java b/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java index 6a5ebb4f343..33f19437ffa 100644 --- a/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java +++ b/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java @@ -175,11 +175,25 @@ public QuorumCnxManager(final long mySid, boolean listenOnAllIPs, int quorumCnxnThreadsSize, boolean quorumSaslAuthEnabled) { + this(mySid, view, authServer, authLearner, socketTimeout, listenOnAllIPs, + quorumCnxnThreadsSize, quorumSaslAuthEnabled, new ConcurrentHashMap()); + } + + // visible for testing + public QuorumCnxManager(final long mySid, + Map view, + QuorumAuthServer authServer, + QuorumAuthLearner authLearner, + int socketTimeout, + boolean listenOnAllIPs, + int quorumCnxnThreadsSize, + boolean quorumSaslAuthEnabled, + ConcurrentHashMap senderWorkerMap) { + this.senderWorkerMap = senderWorkerMap; + this.recvQueue = new ArrayBlockingQueue(RECV_CAPACITY); this.queueSendMap = new ConcurrentHashMap>(); - this.senderWorkerMap = new ConcurrentHashMap(); this.lastMessageSent = new ConcurrentHashMap(); - String cnxToValue = System.getProperty("zookeeper.cnxTimeout"); if(cnxToValue != null){ this.cnxTO = new Integer(cnxToValue); diff --git a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java index 05ff98939cc..e6ecdd1895c 100644 --- a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java +++ b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java @@ -34,6 +34,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.SynchronousQueue; import java.util.concurrent.ThreadPoolExecutor; @@ -65,9 +66,17 @@ import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; +import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyLong; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.verify; + public class QuorumCnxManagerTest extends ZKTestCase { private static final Logger LOG = LoggerFactory.getLogger(QuorumCnxManagerTest.class); private int count; @@ -222,15 +231,21 @@ public void testNoAuthLearnerConnectToAuthRequiredServerWithLowerSid() @Test(timeout = 30000) public void testNoAuthLearnerConnectToAuthRequiredServerWithHigherSid() throws Exception { - QuorumCnxManager peer0 = createAndStartManager(0); - QuorumCnxManager peer1 = createAndStartManager(1, "QuorumServer", - "QuorumLearner", true, true); + ConcurrentHashMap senderWorkerMap0 = + Mockito.spy(new ConcurrentHashMap()); + ConcurrentHashMap senderWorkerMap1 = + Mockito.spy(new ConcurrentHashMap()); + + QuorumCnxManager peer0 = createAndStartManager(0, senderWorkerMap0); + QuorumCnxManager peer1 = createAndStartManager(1, "QuorumServer", "QuorumLearner", + true, true, senderWorkerMap1); peer0.connectOne(1); peer1.connectOne(0); - peer0.halt(); + verify(senderWorkerMap0, timeout(10000)).put(eq(1L), any(QuorumCnxManager.SendWorker.class)); + verify(senderWorkerMap0, timeout(10000)).remove(eq(1L), any(QuorumCnxManager.SendWorker.class)); - assertEventuallyNotConnected(peer0, 1); + verify(senderWorkerMap1, never()).put(anyLong(), any(QuorumCnxManager.SendWorker.class)); } /** @@ -682,9 +697,14 @@ public void testNullQuorumAuthServerWithValidQuorumAuthPacket() } private QuorumCnxManager createAndStartManager(long sid) { + return createAndStartManager(sid, new ConcurrentHashMap()); + } + + private QuorumCnxManager createAndStartManager(long sid, ConcurrentHashMap senderWorkerMap) { QuorumCnxManager peer = new QuorumCnxManager(sid, peers, new NullQuorumAuthServer(), new NullQuorumAuthLearner(), 10000, - false, quorumCnxnThreadsSize, false); + false, quorumCnxnThreadsSize, false, + senderWorkerMap); executor.submit(peer.listener); InetSocketAddress electionAddr = peer.view.get(sid).electionAddr; waitForElectionAddrBinding(electionAddr, 15); @@ -695,14 +715,24 @@ private QuorumCnxManager createAndStartManager(long sid, String serverLoginContext, String learnerLoginContext, boolean serverRequireSasl, - boolean learnerRequireSasl) + boolean learnerRequireSasl) throws Exception { + return createAndStartManager(sid, serverLoginContext, learnerLoginContext, serverRequireSasl, learnerRequireSasl, new ConcurrentHashMap()); + + } + + private QuorumCnxManager createAndStartManager(long sid, + String serverLoginContext, + String learnerLoginContext, + boolean serverRequireSasl, + boolean learnerRequireSasl, + ConcurrentHashMap senderWorkerMap) throws Exception { QuorumAuthLearner authClient = new SaslQuorumAuthLearner(learnerRequireSasl, "NOT_USING_KRB_PRINCIPAL", learnerLoginContext); QuorumAuthServer authServer = new SaslQuorumAuthServer(serverRequireSasl, serverLoginContext, authzHosts); QuorumCnxManager peer = new QuorumCnxManager(sid, peers, - authServer, authClient, 10000, false, quorumCnxnThreadsSize, true); + authServer, authClient, 10000, false, quorumCnxnThreadsSize, true, senderWorkerMap); executor.submit(peer.listener); InetSocketAddress electionAddr = peer.view.get(sid).electionAddr; waitForElectionAddrBinding(electionAddr, 15); From 55869fac9d0f99eb52156c889e1b5a76424047f6 Mon Sep 17 00:00:00 2001 From: Abraham Fine Date: Thu, 27 Apr 2017 13:05:00 -0700 Subject: [PATCH 3/4] add some comments --- .../server/quorum/QuorumCnxManagerTest.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java index e6ecdd1895c..ecf59acf30b 100644 --- a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java +++ b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java @@ -66,7 +66,6 @@ import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; -import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -74,6 +73,7 @@ import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; @@ -191,9 +191,9 @@ public void testClientAuthAgainstNoAuthServerWithLowerSid() /** * Peer0 has auth configured, Peer1 has no auth configured. - * Peer1 connects to peer0, but is disconnected, because peer1's sid is + * Peer0 connects to peer1, but is disconnected, because peer1's sid is * higher than peer0. - * Peer0 connects to peer1, but is disconnected, because peer1 cannot + * Peer1 connects to peer0, but is disconnected, because peer1 cannot * handle auth. */ @Test(timeout = 30000) @@ -227,14 +227,23 @@ public void testNoAuthLearnerConnectToAuthRequiredServerWithLowerSid() * No auth learner connects to a server that requires auth, when the server * has a higher sid. * The connection should fail in both directions. + * + * peer0 should attempt to connect to peer1, but disconnect as its sid is lower + * peer1 should attempt to connect to peer0, peer0 will accept and add an entry to + * the senderWorkerMap but peer1 will disconnect because peer1 will start speaking SASL + * and peer0 will consider this invalid. + * + * Due to the unique behavior of peer0 creating an entry + * in senderWorkerMap for peer1 and then deleting it we use mockito spies to track + * this behavior. */ @Test(timeout = 30000) public void testNoAuthLearnerConnectToAuthRequiredServerWithHigherSid() throws Exception { ConcurrentHashMap senderWorkerMap0 = - Mockito.spy(new ConcurrentHashMap()); + spy(new ConcurrentHashMap()); ConcurrentHashMap senderWorkerMap1 = - Mockito.spy(new ConcurrentHashMap()); + spy(new ConcurrentHashMap()); QuorumCnxManager peer0 = createAndStartManager(0, senderWorkerMap0); QuorumCnxManager peer1 = createAndStartManager(1, "QuorumServer", "QuorumLearner", @@ -242,6 +251,8 @@ public void testNoAuthLearnerConnectToAuthRequiredServerWithHigherSid() peer0.connectOne(1); peer1.connectOne(0); + assertEventuallyNotConnected(peer0, 1); + verify(senderWorkerMap0, timeout(10000)).put(eq(1L), any(QuorumCnxManager.SendWorker.class)); verify(senderWorkerMap0, timeout(10000)).remove(eq(1L), any(QuorumCnxManager.SendWorker.class)); From 4e1216e67023a3615cd837af80f90fe3c44283b8 Mon Sep 17 00:00:00 2001 From: Abraham Fine Date: Thu, 27 Apr 2017 19:09:51 -0700 Subject: [PATCH 4/4] Remove extra assertion accidentally added in previous commit --- .../apache/zookeeper/server/quorum/QuorumCnxManagerTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java index ecf59acf30b..9917cfc7388 100644 --- a/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java +++ b/src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java @@ -251,8 +251,6 @@ public void testNoAuthLearnerConnectToAuthRequiredServerWithHigherSid() peer0.connectOne(1); peer1.connectOne(0); - assertEventuallyNotConnected(peer0, 1); - verify(senderWorkerMap0, timeout(10000)).put(eq(1L), any(QuorumCnxManager.SendWorker.class)); verify(senderWorkerMap0, timeout(10000)).remove(eq(1L), any(QuorumCnxManager.SendWorker.class));