This is an automated email from the ASF dual-hosted git repository.
maoling pushed a commit to branch branch-3.6
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/branch-3.6 by this push:
new 62a7572a5 ZOOKEEPER-4308: Fix flaky test EagerACLFilterTest
62a7572a5 is described below
commit 62a7572a541c0ff4ea74b03d396b78b191a423e6
Author: Kezhu Wang <[email protected]>
AuthorDate: Sun Sep 4 16:49:49 2022 +0800
ZOOKEEPER-4308: Fix flaky test EagerACLFilterTest
There are several problems in this test:
* It uses `ParameterizedTest` which run tests in single jvm. But
`ZooKeeperServer.enableEagerACLCheck` is `static` and loaded from env
variable.
* It uses `assertNotSame` which assert on object reference equiality.
* It asserts on `zkLeader.getLastLoggedZxid()` while client connect to
`connectedServer`. There is no happen-before between
`zkLeader.getLastLoggedZxid()` and successful response from other
server. The commit and response are routed to different servers and
performed asynchronous in each server.
Author: Kezhu Wang <kezhuwgmail.com>
Reviewers: maoling <maoling199210191sina.com>, Mate Szalay-Beko
<symatapache.org>
Closes #1851 from kezhuw/ZOOKEEPER-4308-EagerACLFilterTest
(cherry picked from commit 794790c9f6cbacf158493867f3058a6de748b54e)
Author: Kezhu Wang <[email protected]>
Reviewers: maoling [email protected], Mate Szalay-Beko
[email protected]
Closes #1901 from kezhuw/ZOOKEEPER-4308-EagerACLFilterTest-branch-3.6
---
.../apache/zookeeper/server/ZooKeeperServer.java | 13 ++-
.../server/quorum/EagerACLFilterTest.java | 98 +++++++++++++++-------
.../java/org/apache/zookeeper/test/QuorumBase.java | 2 +-
3 files changed, 82 insertions(+), 31 deletions(-)
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
index 16b62db22..7adfb9e12 100644
---
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
+++
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
@@ -102,7 +102,7 @@ public class ZooKeeperServer implements SessionExpirer,
ServerStats.Provider {
// When enabled, will check ACL constraints appertained to the requests
first,
// before sending the requests to the quorum.
- static final boolean enableEagerACLCheck;
+ static boolean enableEagerACLCheck;
static final boolean skipACL;
@@ -141,6 +141,17 @@ public class ZooKeeperServer implements SessionExpirer,
ServerStats.Provider {
LOG.info("{} = {}", CLOSE_SESSION_TXN_ENABLED, closeSessionTxnEnabled);
}
+ // @VisibleForTesting
+ public static boolean isEnableEagerACLCheck() {
+ return enableEagerACLCheck;
+ }
+
+ // @VisibleForTesting
+ public static void setEnableEagerACLCheck(boolean enabled) {
+ ZooKeeperServer.enableEagerACLCheck = enabled;
+ LOG.info("Update {} to {}", ENABLE_EAGER_ACL_CHECK, enabled);
+ }
+
public static boolean isCloseSessionTxnEnabled() {
return closeSessionTxnEnabled;
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/EagerACLFilterTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/EagerACLFilterTest.java
index fa18bf0e2..c059e3af7 100644
---
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/EagerACLFilterTest.java
+++
b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/EagerACLFilterTest.java
@@ -19,15 +19,17 @@
package org.apache.zookeeper.server.quorum;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import java.util.Arrays;
import java.util.Collection;
-import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.CompletableFuture;
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.KeeperException;
import org.apache.zookeeper.TestableZooKeeper;
import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.server.ZooKeeperServer;
import org.apache.zookeeper.server.quorum.QuorumPeer.ServerState;
import org.apache.zookeeper.test.QuorumBase;
@@ -42,7 +44,6 @@ public class EagerACLFilterTest extends QuorumBase {
protected boolean checkEnabled;
protected ServerState serverState;
- protected final CountDownLatch callComplete = new CountDownLatch(1);
protected boolean complete = false;
protected static final String PARENT_PATH = "/foo";
protected static final String CHILD_PATH = "/foo/bar";
@@ -52,7 +53,9 @@ public class EagerACLFilterTest extends QuorumBase {
protected static final byte[] DATA = "Hint Water".getBytes();
protected TestableZooKeeper zkClient;
protected TestableZooKeeper zkClientB;
+ protected TestableZooKeeper zkLeaderClient;
protected QuorumPeer zkLeader;
+ protected QuorumPeer zkConnected;
protected ZooKeeperServer connectedServer;
@Parameterized.Parameters
@@ -68,6 +71,7 @@ public class EagerACLFilterTest extends QuorumBase {
@Before
public void setUp() throws Exception {
ensureCheck(checkEnabled);
+ CountdownWatcher leaderWatch = new CountdownWatcher();
CountdownWatcher clientWatch = new CountdownWatcher();
CountdownWatcher clientWatchB = new CountdownWatcher();
super.setUp(true);
@@ -76,16 +80,31 @@ public class EagerACLFilterTest extends QuorumBase {
int clientPort = Integer.parseInt(hostPort.split(":")[1]);
zkLeader = getPeerList().get(getLeaderIndex());
- connectedServer = getPeerByClientPort(clientPort).getActiveServer();
+ zkConnected = getPeerByClientPort(clientPort);
+ connectedServer = zkConnected.getActiveServer();
+ zkLeaderClient = createClient(leaderWatch,
getPeersMatching(ServerState.LEADING));
zkClient = createClient(clientWatch, hostPort);
zkClientB = createClient(clientWatchB, hostPort);
zkClient.addAuthInfo(AUTH_PROVIDER, AUTH);
zkClientB.addAuthInfo(AUTH_PROVIDER, AUTHB);
+ leaderWatch.waitForConnected(CONNECTION_TIMEOUT);
clientWatch.waitForConnected(CONNECTION_TIMEOUT);
clientWatchB.waitForConnected(CONNECTION_TIMEOUT);
}
+ void syncClient(ZooKeeper zk) {
+ CompletableFuture<Void> synced = new CompletableFuture<>();
+ zk.sync("/", (rc, path, ctx) -> {
+ if (rc == 0) {
+ synced.complete(null);
+ } else {
+
synced.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc)));
+ }
+ }, null);
+ synced.join();
+ }
+
@After
public void tearDown() throws Exception {
if (zkClient != null) {
@@ -100,19 +119,29 @@ public class EagerACLFilterTest extends QuorumBase {
}
private void ensureCheck(boolean enabled) {
- if (enabled) {
- System.setProperty(ZooKeeperServer.ENABLE_EAGER_ACL_CHECK, "true");
- } else {
- System.clearProperty(ZooKeeperServer.ENABLE_EAGER_ACL_CHECK);
- }
+ ZooKeeperServer.setEnableEagerACLCheck(enabled);
}
- private void assertTransactionState(String condition, long lastxid) {
- String assertion = String.format("Server State: %s Check Enabled: %s
%s", serverState, checkEnabled, condition);
- if (checkEnabled) {
- assertEquals(assertion, lastxid, zkLeader.getLastLoggedZxid());
+ private void assertTransactionState(String operation, QuorumPeer peer,
long lastxid) {
+ if (peer == zkLeader && peer != zkConnected) {
+ // The operation is performed on no leader, but we are asserting
on leader.
+ // There is no happen-before between
`zkLeader.getLastLoggedZxid()` and
+ // successful response from other server. The commit and response
are routed
+ // to different servers and performed asynchronous in each server.
So we have
+ // to sync leader client to go through commit and response path in
leader to
+ // build happen-before between `zkLeader.getLastLoggedZxid()` and
side effect
+ // of previous operation.
+ syncClient(zkLeaderClient);
+ }
+ assertTrue(peer == zkLeader || peer == zkConnected);
+ boolean eagerACL = ZooKeeperServer.isEnableEagerACLCheck();
+ String assertion = String.format(
+ "Connecting: %s Checking: %s EagerACL: %s Operation: %s",
+ zkConnected.getPeerState(), peer.getPeerState(), eagerACL,
operation);
+ if (eagerACL) {
+ assertEquals(assertion, lastxid, peer.getLastLoggedZxid());
} else {
- assertNotSame(assertion, lastxid, zkLeader.getLastLoggedZxid());
+ assertNotEquals(assertion, lastxid, peer.getLastLoggedZxid());
}
}
@@ -136,29 +165,33 @@ public class EagerACLFilterTest extends QuorumBase {
@Test
public void testCreateFail() throws Exception {
zkClient.create(PARENT_PATH, DATA, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
- long lastxid = zkLeader.getLastLoggedZxid();
+ long lastxid = zkConnected.getLastLoggedZxid();
try {
zkClientB.create(CHILD_PATH, DATA, Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+ fail("expect no auth");
} catch (KeeperException.NoAuthException e) {
}
assertEquals("OutstandingRequests not decremented", 0,
connectedServer.getInProcess());
- assertTransactionState("Transaction state on Leader after failed
create", lastxid);
+ assertTransactionState("failed create", zkConnected, lastxid);
+ assertTransactionState("failed create", zkLeader, lastxid);
}
@Test
public void testCreate2Fail() throws Exception {
zkClient.create(PARENT_PATH, DATA, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT, null);
- long lastxid = zkLeader.getLastLoggedZxid();
+ long lastxid = zkConnected.getLastLoggedZxid();
try {
zkClientB.create(CHILD_PATH, DATA, Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT, null);
+ fail("expect no auth");
} catch (KeeperException.NoAuthException e) {
}
assertEquals("OutstandingRequests not decremented", 0,
connectedServer.getInProcess());
- assertTransactionState("Transaction state on Leader after failed
create2", lastxid);
+ assertTransactionState("failed create2", zkConnected, lastxid);
+ assertTransactionState("failed create2", zkLeader, lastxid);
}
@Test
@@ -173,15 +206,17 @@ public class EagerACLFilterTest extends QuorumBase {
public void testDeleteFail() throws Exception {
zkClient.create(PARENT_PATH, DATA, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT, null);
zkClient.create(CHILD_PATH, DATA, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT, null);
- long lastxid = zkLeader.getLastLoggedZxid();
+ long lastxid = zkConnected.getLastLoggedZxid();
try {
zkClientB.delete(CHILD_PATH, -1);
+ fail("expect no auth");
} catch (KeeperException.NoAuthException e) {
}
assertEquals("OutstandingRequests not decremented", 0,
connectedServer.getInProcess());
- assertTransactionState("Transaction state on Leader after failed
delete", lastxid);
+ assertTransactionState("failed delete", zkConnected, lastxid);
+ assertTransactionState("failed delete", zkLeader, lastxid);
}
@Test
@@ -193,15 +228,17 @@ public class EagerACLFilterTest extends QuorumBase {
@Test
public void testSetDataFail() throws Exception {
zkClient.create(PARENT_PATH, null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT, null);
- long lastxid = zkLeader.getLastLoggedZxid();
+ long lastxid = zkConnected.getLastLoggedZxid();
try {
zkClientB.setData(PARENT_PATH, DATA, -1);
+ fail("expect no auth");
} catch (KeeperException.NoAuthException e) {
}
assertEquals("OutstandingRequests not decremented", 0,
connectedServer.getInProcess());
- assertTransactionState("Transaction state on Leader after failed
setData", lastxid);
+ assertTransactionState("failed setData", zkConnected, lastxid);
+ assertTransactionState("failed setData", zkLeader, lastxid);
}
@Test
@@ -215,26 +252,28 @@ public class EagerACLFilterTest extends QuorumBase {
@Test
public void testSetACLFail() throws Exception {
zkClient.create(PARENT_PATH, null, Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT, null);
- long lastxid = zkLeader.getLastLoggedZxid();
+ long lastxid = zkConnected.getLastLoggedZxid();
try {
zkClientB.setACL(PARENT_PATH, Ids.READ_ACL_UNSAFE, -1);
- } catch (KeeperException.NoAuthException e) {
+ fail("expect no auth");
+ } catch (KeeperException.NoAuthException ignored) {
}
assertEquals("OutstandingRequests not decremented", 0,
connectedServer.getInProcess());
- assertTransactionState("Transaction state on Leader after failed
setACL", lastxid);
+ assertTransactionState("failed setACL", zkConnected, lastxid);
+ assertTransactionState("failed setACL", zkLeader, lastxid);
}
@Test
public void testBadACL() throws Exception {
CountdownWatcher cw = new CountdownWatcher();
- TestableZooKeeper zk = createClient(cw, getPeersMatching(serverState));
- long lastxid;
+ String addr = String.format("%s:%d", LOCALADDR,
zkConnected.getClientPort());
+ TestableZooKeeper zk = createClient(cw, addr);
cw.waitForConnected(CONNECTION_TIMEOUT);
- lastxid = zkLeader.getLastLoggedZxid();
+ long lastxid = zkConnected.getLastLoggedZxid();
try {
zk.create("/acltest", new byte[0], Ids.CREATOR_ALL_ACL,
CreateMode.PERSISTENT);
@@ -244,7 +283,8 @@ public class EagerACLFilterTest extends QuorumBase {
assertEquals("OutstandingRequests not decremented", 0,
connectedServer.getInProcess());
- assertTransactionState("zxid after invalid ACL", lastxid);
+ assertTransactionState("invalid ACL", zkConnected, lastxid);
+ assertTransactionState("invalid ACL", zkLeader, lastxid);
}
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuorumBase.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuorumBase.java
index b91aa19e9..672304b90 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuorumBase.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/QuorumBase.java
@@ -45,7 +45,7 @@ public class QuorumBase extends ClientBase {
private static final Logger LOG =
LoggerFactory.getLogger(QuorumBase.class);
- private static final String LOCALADDR = "127.0.0.1";
+ protected static final String LOCALADDR = "127.0.0.1";
File s1dir, s2dir, s3dir, s4dir, s5dir;
QuorumPeer s1, s2, s3, s4, s5;
