You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/09/02 22:45:34 UTC
[GitHub] [zookeeper] hanm opened a new pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
hanm opened a new pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445
Test case only for now, submit for community feedback. Don't merge please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-823889471
Hi @fanyang89,
> > I started looking into the issue with 3.5, but haven't had a chance to figure out the inconsistency. In case it helps, here is my current tree:
>
> @ztzg ZOOKEEPER-3911 requires ZOOKEEPER-3240 to pass the unit test.
Sorry for the lag, and thank you very much for investigating/clarifying this!
> I've created a new PR to backport ZOOKEEPER-3911 to branch-3.5.9.
>
> #1655
Thank you—but as pointed out by @anmolnar in https://github.com/apache/zookeeper/pull/1655#issuecomment-808795773, `branch-3.5.9` is a "release branch," and 3.5.9 has already been released. Could you please submit the same PR against `branch-3.5` (the "series" branch)?
(I will hopefully have a bit more time to look into these topics in the coming weeks.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm closed pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm closed pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731430407
@hanm @li4wang @tamaashu Yep, I also tried but since there were commits during those jUnit 5 upgrade, there are conflicts if we cherry-pick those upgrade commits. It's easier to fix it directly if we don't have more backports to branch 3.6.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-691722471
retest maven build
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tamaashu commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
tamaashu commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731396176
@li4wang ZOOKEEPER-3850 won't solve the issue, since it only replaces jUnit4 with jUnit5-vintage (which is just a jUnit 4.13 with a rename). I would advise to fix the testcases in branch 3.6 with the use of jUnit4.
ZooKeeper 3.7 is coming with fully supported jUnit5 testing, there won't be too many backports to 3.6 afterwards.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-686629954
One more observation - I think we have the same issue even before ZOOKEEPER-2678 (where it avoids taking snapshot before sending NEWLEADER ACK back during DIFF sync), because even when the snapshot was taken, the data tree at that time still does not have the proposals DIFF sync from leader applied (the commit of all such proposals happen after follower receive UPTODATE).
Specifically, in zab paper, it said:
"Phase 2: Sync with followers:
f The follower syncs with the leader, but doesn't modify its state until it receives the NEWLEADER(e) packet. Once it receives NEWLEADER(e) it atomically applies the new state and sets f.currentEpoch = e. It then sends ACK(e << 32)."
But in our implementation, I don't see we actually "atomically applies the new state" before sends ACK.
@breed might have more context around this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] anmolnar commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
anmolnar commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-808796332
@fanyang89 Thanks for taking care of this. Please open PR against `branch-3.5`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-726137075
Merged to master, branch-3.6 and branch-3.5. Thanks @hanm
3.5 did not pick clean, I fixed up manually, I will do some testing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731338543
@pkuwm yes, I am fixing it. I am planning to back porting two commits from master branch to branch-3.6 via cherry-picking.
8da9c723a ZOOKEEPER-3850: Update jUnit to 5.6 in pom.xml files
c42c8c940 ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm closed pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm closed pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-686626410
>> I see there are a lot of "Thread.sleep", this could lead to flakes
I think it's fine here as the execution does not depends on timing of the sleep. The functionality these code block is basically the same with the waitForAll/One helpers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r483860051
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -662,6 +662,20 @@ public void startdata() throws IOException, InterruptedException {
}
public synchronized void startup() {
+ startupWithServerState(State.RUNNING);
+ notifyAll();
+ }
+
+ public synchronized void startupWithoutServing() {
+ startupWithServerState(State.INITIAL);
+ notifyAll();
+ }
+
+ public synchronized void startServing() {
+ setState(State.RUNNING);
Review comment:
good catch - there is no correctness issue here without notifying because the wait will timeout anyway and next check will catch the correct state, but better to notify even the timeout is pretty short to avoid unnecessary wait.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-730702728
@hanm @nkalmar I am getting the following compilation error in branch-3.6. The error started to occur after this commit. I wonder if you ran into the same issue. Can you please take a look at it?
====================
[INFO] Finished at: 2020-11-19T15:07:24-08:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:testCompile (default-testCompile) on project zookeeper: Compilation failure: Compilation failure:
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[22,36] package org.junit.jupiter.api does not exist
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[22,1] static import only from classes and interfaces
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[23,36] package org.junit.jupiter.api does not exist
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[23,1] static import only from classes and interfaces
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[24,36] package org.junit.jupiter.api does not exist
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[24,1] static import only from classes and interfaces
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[25,36] package org.junit.jupiter.api does not exist
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[25,1] static import only from classes and interfaces
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[42,29] package org.junit.jupiter.api does not exist
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[43,29] package org.junit.jupiter.api does not exist
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[44,29] package org.junit.jupiter.api does not exist
[ERROR] /Users/liwang/apache/zookeeper/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:[51,6] cannot find symbol
[ERROR] symbol: class Test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r483382118
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
##########
@@ -741,18 +741,30 @@ protected void syncWithLeader(long newLeaderZxid) throws Exception {
}
self.setCurrentEpoch(newEpoch);
- writeToTxnLog = true; //Anything after this needs to go to the transaction log, not applied directly in memory
+ writeToTxnLog = true;
+ //Anything after this needs to go to the transaction log, not applied directly in memory
isPreZAB1_0 = false;
+
+ // ZOOKEEPER-3911: make sure sync the uncommitted logs before commit them (ACK NEWLEADER).
+ sock.setSoTimeout(self.tickTime * self.syncLimit);
+ self.setSyncMode(QuorumPeer.SyncMode.NONE);
+ zk.startup();
Review comment:
Add a startServing api to address this issue - follower will only start serving after receiving UPTODATE (meaning leader should already have received quorum NEWLEADER ACK).
I'll add some test cases around the startServing api if this looks good to reviewers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r482753918
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java
##########
@@ -0,0 +1,293 @@
+/*
+ * 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 static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.io.IOException;
+import java.net.SocketTimeoutException;
+import java.util.Map;
+import javax.security.sasl.SaslException;
+import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooDefs.OpCode;
+import org.apache.zookeeper.ZooKeeper;
+import org.apache.zookeeper.ZooKeeper.States;
+import org.apache.zookeeper.data.Stat;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.apache.zookeeper.server.quorum.Leader.Proposal;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.test.ClientBase.CountdownWatcher;
+
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+
+public class DIFFSyncConsistencyTest extends QuorumPeerTestBase {
+
+ private static int SERVER_COUNT = 3;
+ private MainThread[] mt = new MainThread[SERVER_COUNT];
+
+ @Test
+ @Timeout(value = 120)
+ public void testInconsistentDueToUncommittedLog() throws Exception {
+ final int LEADER_TIMEOUT_MS = 10_000;
+ final int[] clientPorts = new int[SERVER_COUNT];
+
+ StringBuilder sb = new StringBuilder();
+ String server;
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ clientPorts[i] = PortAssignment.unique();
+ server = "server." + i + "=127.0.0.1:" + PortAssignment.unique() + ":" + PortAssignment.unique()
+ + ":participant;127.0.0.1:" + clientPorts[i];
+ sb.append(server + "\n");
+ }
+ String currentQuorumCfgSection = sb.toString();
+
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ mt[i] = new MainThread(i, clientPorts[i], currentQuorumCfgSection, false) {
+ @Override
+ public TestQPMain getTestQPMain() {
+ return new MockTestQPMain();
+ }
+ };
+ mt[i].start();
+ }
+
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ assertTrue(ClientBase.waitForServerUp("127.0.0.1:" + clientPorts[i], CONNECTION_TIMEOUT),
+ "waiting for server " + i + " being up");
+ }
+
+ int leader = findLeader(mt);
+ CountdownWatcher watch = new CountdownWatcher();
+ ZooKeeper zk = new ZooKeeper("127.0.0.1:" + clientPorts[leader], ClientBase.CONNECTION_TIMEOUT, watch);
+ watch.waitForConnected(ClientBase.CONNECTION_TIMEOUT);
+
+ Map<Long, Proposal> outstanding = mt[leader].main.quorumPeer.leader.outstandingProposals;
+ // Increase the tick time to delay the leader going to looking to allow us proposal a transaction while other
+ // followers are offline.
+ int previousTick = mt[leader].main.quorumPeer.tickTime;
+ mt[leader].main.quorumPeer.tickTime = LEADER_TIMEOUT_MS;
+ // Let the previous tick on the leader exhaust itself so the new tick time takes effect
+ Thread.sleep(previousTick);
+
+ LOG.info("LEADER ELECTED {}", leader);
+
+ // Shutdown followers to make sure we don't accidentally send the proposal we are going to make to follower.
+ // In other words, we want to make sure the followers get the proposal later through DIFF sync.
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ if (i != leader) {
+ mt[i].shutdown();
+ }
+ }
+
+ // Send a create request to old leader and make sure it's synced to disk.
+ try {
+ zk.create("/zk" + leader, "zk".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ fail("create /zk" + leader + " should have failed");
+ } catch (KeeperException e) {
+ }
+
+ // Make sure that we actually did get it in process at the leader; there can be extra sessionClose proposals.
+ assertTrue(outstanding.size() > 0);
+ Proposal p = findProposalOfType(outstanding, OpCode.create);
+ LOG.info("Old leader id: {}. All proposals: {}", leader, outstanding);
+ assertNotNull(p, "Old leader doesn't have 'create' proposal");
+
+ // Make sure leader sync the proposal to disk.
+ int sleepTime = 0;
+ Long longLeader = (long) leader;
+ while (!p.qvAcksetPairs.get(0).getAckset().contains(longLeader)) {
+ if (sleepTime > 2000) {
+ fail("Transaction not synced to disk within 1 second " + p.qvAcksetPairs.get(0).getAckset() + " expected " + leader);
+ }
+ Thread.sleep(100);
+ sleepTime += 100;
+ }
+
+ // Start controlled followers where we deliberately make the follower fail once follower receive the UPTODATE
+ // message from leader. Because followers only persist proposals from DIFF sync after UPTODATE, this can
+ // deterministically simulate the situation where followers ACK NEWLEADER (which makes leader think she has the
+ // quorum support, but actually not afterwards) but immediately fail afterwards without persisting the proposals
+ // from DIFF sync.
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ if (i == leader) {
+ continue;
+ }
+
+ mt[i].start();
+ int sleepCount = 0;
+ while (mt[i].getQuorumPeer() == null) {
+ ++sleepCount;
+ if (sleepCount > 100) {
+ fail("Can't start follower " + i + " !");
+ }
+ Thread.sleep(100);
+ }
+
+ ((CustomQuorumPeer) mt[i].getQuorumPeer()).setInjectError(true);
+ LOG.info("Follower {} started.", i);
+ }
+
+ // Verify leader can see it. The fact that leader can see it implies that
+ // leader should, at this point in time, get a quorum of ACK of NEWLEADER
+ // from two followers so leader can start serving requests; this also implies
+ // that DIFF sync from leader to followers are finished at this point in time.
+ // We then verify later that followers should have the same view after we shutdown
+ // this leader, otherwise it's a violation of ZAB / sequential consistency.
+ int c = 0;
+ while (c < 100) {
+ ++c;
+ try {
+ Stat stat = zk.exists("/zk" + leader, false);
+ assertNotNull(stat, "server " + leader + " should have /zk");
+ break;
+ } catch (KeeperException.ConnectionLossException e) {
+
+ }
+ Thread.sleep(100);
+ }
+
+ // Shutdown all servers
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ mt[i].shutdown();
+ }
+ waitForOne(zk, States.CONNECTING);
+
+ // Now restart all servers except the old leader. Only old leader has the transaction sync to disk.
+ // The old followers only had in memory view of the transaction, and they didn't have a chance
+ // to sync to disk because we made them fail at UPTODATE.
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ if (i == leader) {
+ continue;
+ }
+ mt[i].start();
+ int sleepCount = 0;
+ while (mt[i].getQuorumPeer() == null) {
+ ++sleepCount;
+ if (sleepCount > 100) {
+ fail("Can't start follower " + i + " !");
+ }
+ Thread.sleep(100);
+ }
+
+ ((CustomQuorumPeer) mt[i].getQuorumPeer()).setInjectError(false);
+ LOG.info("Follower {} started again.", i);
+ }
+
+ int newLeader = findLeader(mt);
+ assertNotEquals(newLeader, leader, "new leader is still the old leader " + leader + " !!");
+
+ // This simulate the case where a ZooKeeper client previously connected to old leader had a view of the data
+ // "/zkX", and now reconnected to remaining quorum members and should also have the same view of the data.
+ // Missing such a view is a violation of ZAB and Sequential Consistency of ZooKeeper.
+ for (int i = 0; i < SERVER_COUNT; i++) {
+ if (i == leader) {
+ continue;
+ }
+ zk.close();
+ zk = new ZooKeeper("127.0.0.1:" + clientPorts[i], ClientBase.CONNECTION_TIMEOUT, watch);
+ watch.waitForConnected(ClientBase.CONNECTION_TIMEOUT);
+ Stat val = zk.exists("/zk" + leader, false);
+ assertNotNull(val, "Data inconsistency detected! " +
+ "Server " + i + " should have a view of /zk" + leader + "!");
+ }
+
+ zk.close();
+ }
+
+ @AfterEach
+ public void tearDown() {
+ for (int i = 0; i < mt.length; i++) {
+ try {
+ mt[i].shutdown();
+ } catch (InterruptedException e) {
+ LOG.warn("Quorum Peer interrupted while shutting it down", e);
+ }
+ }
+ }
+
+ static class CustomQuorumPeer extends QuorumPeer {
+
+ private boolean injectError = false;
Review comment:
probably this should be `volatile`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-724086688
Since this has 2 committer's +1 I'm keen on merging this. I'd say to master, 3.6 and even 3.5, hopefully cherry picks clean.
I will wait another day if any reactions comes, and then I'll merge (or ofc others can merge before that).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731401566
@tamaashu yeah, I noticed ZOOKEEPER-3850 by itself is not sufficient. I fixed the compilation errors by applying ZOOKEEPER-3850 and some addition changes on pom.xml, but the test case failed too.
@hanm can you please fix the test failure along with the compilation error. I do agree with @tamaashu, it would be better just fix the testcases in branch 3.6 by not using JUnit5 API.
test case error
=========
[INFO] Running org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 30.442 s <<< FAILURE! - in org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] testInconsistentDueToUncommittedLog Time elapsed: 30.326 s <<< FAILURE!
org.opentest4j.AssertionFailedError: waiting for server 0 being up ==> expected: <true> but was: <false>
at org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest.testInconsistentDueToUncommittedLog(DIFFSyncConsistencyTest.java:78)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r512895762
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
##########
@@ -741,18 +741,30 @@ protected void syncWithLeader(long newLeaderZxid) throws Exception {
}
self.setCurrentEpoch(newEpoch);
- writeToTxnLog = true; //Anything after this needs to go to the transaction log, not applied directly in memory
+ writeToTxnLog = true;
+ //Anything after this needs to go to the transaction log, not applied directly in memory
isPreZAB1_0 = false;
+
+ // ZOOKEEPER-3911: make sure sync the uncommitted logs before commit them (ACK NEWLEADER).
+ sock.setSoTimeout(self.tickTime * self.syncLimit);
+ self.setSyncMode(QuorumPeer.SyncMode.NONE);
+ zk.startupWithoutServing();
+ if (zk instanceof FollowerZooKeeperServer) {
+ FollowerZooKeeperServer fzk = (FollowerZooKeeperServer) zk;
+ for (PacketInFlight p : packetsNotCommitted) {
+ fzk.logRequest(p.hdr, p.rec, p.digest);
+ }
+ packetsNotCommitted.clear();
+ }
+
writePacket(new QuorumPacket(Leader.ACK, newLeaderZxid, null, null), true);
break;
}
}
}
ack.setZxid(ZxidUtils.makeZxid(newEpoch, 0));
writePacket(ack, true);
- sock.setSoTimeout(self.tickTime * self.syncLimit);
- self.setSyncMode(QuorumPeer.SyncMode.NONE);
- zk.startup();
+ zk.startServing();
Review comment:
>> but in other code paths we are not calling "startupWithoutServing"
The invariant here is that a call to ` zk.startServing` will always follow a call to `zk.startupWithoutServing`. This is because the ZAB protocol implementation requires this order of invocation (in `syncWithLeader`). In other words, there is no other code paths that involve both calls. To finish the sync, the leader and follower must follow these events in order.
* First, learner will receive `NEWLEADER` from leader. This is where we will call `zk.startupWithoutServing`.
* Then, learner will receive `UPTODATE` from leader. This is where we breaks the outer sync loop, afterwards we call `zk.startServing`.
Hope this make sense. A more detailed description can be found on phase 2 "Sync with followers" on https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zab1.0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] fanyang89 commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
fanyang89 commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-823895681
Hi @ztzg, I've created a new PR against branch-3.5 https://github.com/apache/zookeeper/pull/1657, but I forget to update the link at this thread. Sorry for that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731338543
@pkuwm yes, I am fixing it. I am planning to back porting two commits from master branch to branch-3.6 via cherry-picking.
63f744b2f ZOOKEEPER-3863: correct junit test timeout
0b386e457 ZOOKEEPER-3862: Re-enable deprecation check after finishing jUnit upg…
08ae49dee ZOOKEEPER-3854: Upgrade jUnit in ZooKeeper-Recipes
531aae3ed ZOOKEEPER-3855: Upgrade jUnit in ZooKeeper-Metrics-providers
c42c8c940 ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
6ab1822ec ZOOKEEPER-3852: Upgrade jUnit in ZooKeeper-Jute
8da9c723a ZOOKEEPER-3850: Update jUnit to 5.6 in pom.xml files
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-727266138
I'm a bit late in reviewing this, but FWIW: LGTM.
I started looking into the issue with 3.5, but haven't had a chance to figure out the inconsistency. In case it helps, here is my current tree:
https://github.com/ztzg/zookeeper/tree/ZOOKEEPER-3911-diff-sync-3.5
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] fregatte123 commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
fregatte123 commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r483390796
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -662,6 +662,20 @@ public void startdata() throws IOException, InterruptedException {
}
public synchronized void startup() {
+ startupWithServerState(State.RUNNING);
+ notifyAll();
+ }
+
+ public synchronized void startupWithoutServing() {
+ startupWithServerState(State.INITIAL);
+ notifyAll();
+ }
+
+ public synchronized void startServing() {
+ setState(State.RUNNING);
Review comment:
notifyAll(); here may be needed
to wakeup the thread sleep on the condition of State.INITIAL.
-------------------------
public void enqueueRequest(Request si)
public void submitRequestNow(Request si)
sleep at State.INITIAL
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-824228862
> Hi @ztzg, I've created a new PR against branch-3.5 #1657, but I forget to update the link at this thread. Sorry for that.
No problem. Now merged in branch `branch-3.5`. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731338543
@pkuwm yes, I am fixing it. I am planning to back porting the following commits from master branch to branch-3.6 via cherry-picking.
63f744b2f ZOOKEEPER-3863: correct junit test timeout
0b386e457 ZOOKEEPER-3862: Re-enable deprecation check after finishing jUnit upg…
08ae49dee ZOOKEEPER-3854: Upgrade jUnit in ZooKeeper-Recipes
531aae3ed ZOOKEEPER-3855: Upgrade jUnit in ZooKeeper-Metrics-providers
c42c8c940 ZOOKEEPER-3872: Upgrade jUnit in ZooKeeper-server
6ab1822ec ZOOKEEPER-3852: Upgrade jUnit in ZooKeeper-Jute
8da9c723a ZOOKEEPER-3850: Update jUnit to 5.6 in pom.xml files
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-727131449
Thanks @nkalmar for taking care of merging this. I'll check out what's the problem with the patch on branch-3.5.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] fregatte123 commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
fregatte123 commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r483357169
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
##########
@@ -741,18 +741,30 @@ protected void syncWithLeader(long newLeaderZxid) throws Exception {
}
self.setCurrentEpoch(newEpoch);
- writeToTxnLog = true; //Anything after this needs to go to the transaction log, not applied directly in memory
+ writeToTxnLog = true;
+ //Anything after this needs to go to the transaction log, not applied directly in memory
isPreZAB1_0 = false;
+
+ // ZOOKEEPER-3911: make sure sync the uncommitted logs before commit them (ACK NEWLEADER).
+ sock.setSoTimeout(self.tickTime * self.syncLimit);
+ self.setSyncMode(QuorumPeer.SyncMode.NONE);
+ zk.startup();
Review comment:
@hanm
According to the code, inconsistencies may occur as follows
step1: Assuming that there are 4 followers and 1 leader at this time, their current maximum zxid is as follows:
follower A:zxid = n
follower B:zxid = n+1
follower C:zxid = n+2
follower D:zxid = n+3
leader E:zxid = n+4
step2: The gap between the synchronization of leader and follower D, such as DIFF
step3: follower D receives the leader’s proposal, puts it in memory, and then calls zk.startup(), which means that the client (such as webapp) can see follower D:zxid = n+3
step4: follower D writes the proposal in memory to disk
step5: follower D writePacket(new QuorumPacket(Leader.ACK, newLeaderZxid, null, null), true). ACK-LD message will be sent to leader(ACK-LD from the paper[1]), and then follower D will be stopped
step6: If leader E is stopped before ACK-L has not reached the quorum. (The leader receives the quorum ACK-LD, which means that the proposals with zxid <= (n+4) have been persisted, which means that no matter whether the leader is down or re-elected or the leader disk is damaged, it cannot be started. The proposal of =n+4 has reached consensus in the cluster.)
step7: The cluster will select the leader from the 3 surviving machines.
A:zxid = n B:zxid = n+1 C:zxid = n+2
Suppose that C becomes the leader (a total of 5 machines, 3 survive, and can reach a quorum).
When A and B are synchronized with C
A:zxid = n+2 B:zxid = n+2 C:zxid = n+2
Conclusion: zxid=n+3 cannot be accessed, resulting in inconsistent access
After the leader receives the quorum ACK-LD, the follower starts the external access service, which is relatively safe. If the follower provides external access before then, the follower cannot guarantee that its zxid has reached the quorum
[1]Zab: High-performance broadcast for primary-backup systems
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-686779752
jenkins pre-merge reports one failed test: FileTxnSnapLogMetricsTest#testFileTxnSnapLogMetrics
however, i can't reproduce it locally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r483361109
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
##########
@@ -741,18 +741,30 @@ protected void syncWithLeader(long newLeaderZxid) throws Exception {
}
self.setCurrentEpoch(newEpoch);
- writeToTxnLog = true; //Anything after this needs to go to the transaction log, not applied directly in memory
+ writeToTxnLog = true;
+ //Anything after this needs to go to the transaction log, not applied directly in memory
isPreZAB1_0 = false;
+
+ // ZOOKEEPER-3911: make sure sync the uncommitted logs before commit them (ACK NEWLEADER).
+ sock.setSoTimeout(self.tickTime * self.syncLimit);
+ self.setSyncMode(QuorumPeer.SyncMode.NONE);
+ zk.startup();
Review comment:
This is possible but unfortunately there is no way afaik to persist the proposals without staring the server in first place. The better approach is to provide a control API (such as ZooKeeperServer#startServing) so we can control when requests will be allowed to come regardless of the servers state (running or not). Maybe there is already such an API / approach but I need sometime to figure this out.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#discussion_r512734631
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
##########
@@ -741,18 +741,30 @@ protected void syncWithLeader(long newLeaderZxid) throws Exception {
}
self.setCurrentEpoch(newEpoch);
- writeToTxnLog = true; //Anything after this needs to go to the transaction log, not applied directly in memory
+ writeToTxnLog = true;
+ //Anything after this needs to go to the transaction log, not applied directly in memory
isPreZAB1_0 = false;
+
+ // ZOOKEEPER-3911: make sure sync the uncommitted logs before commit them (ACK NEWLEADER).
+ sock.setSoTimeout(self.tickTime * self.syncLimit);
+ self.setSyncMode(QuorumPeer.SyncMode.NONE);
+ zk.startupWithoutServing();
+ if (zk instanceof FollowerZooKeeperServer) {
+ FollowerZooKeeperServer fzk = (FollowerZooKeeperServer) zk;
+ for (PacketInFlight p : packetsNotCommitted) {
+ fzk.logRequest(p.hdr, p.rec, p.digest);
+ }
+ packetsNotCommitted.clear();
+ }
+
writePacket(new QuorumPacket(Leader.ACK, newLeaderZxid, null, null), true);
break;
}
}
}
ack.setZxid(ZxidUtils.makeZxid(newEpoch, 0));
writePacket(ack, true);
- sock.setSoTimeout(self.tickTime * self.syncLimit);
- self.setSyncMode(QuorumPeer.SyncMode.NONE);
- zk.startup();
+ zk.startServing();
Review comment:
One question:
this "startServing" works if you executed 'zk.startupWithoutServing();' in your new code.
but in other code paths we are not calling "startupWithoutServing"
so when we come to this point zk.startServing() will only change the state to RUNNING, and we had not called 'startupWithServerState'
can you please explain ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] fanyang89 commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
fanyang89 commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-808158462
> I started looking into the issue with 3.5, but haven't had a chance to figure out the inconsistency. In case it helps, here is my current tree:
@ztzg ZOOKEEPER-3911 requires ZOOKEEPER-3240 to pass the unit test. I've created a new PR to backport ZOOKEEPER-3911 to branch-3.5.9.
https://github.com/apache/zookeeper/pull/1655
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731401566
@tamaashu yeah, I noticed ZOOKEEPER-3850 by itself is not sufficient. I fixed the compilation errors by applying ZOOKEEPER-3850 and some addition changes on pom.xml, but the test case failed too.
@hanm can you please fix the test failure along with the compilation error? I do agree with @tamaashu, it would be better just fix the testcases in branch 3.6 by not using JUnit5 API.
test case error
--------------
[INFO] Running org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 30.442 s <<< FAILURE! - in org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] testInconsistentDueToUncommittedLog Time elapsed: 30.326 s <<< FAILURE!
org.opentest4j.AssertionFailedError: waiting for server 0 being up ==> expected: <true> but was: <false>
at org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest.testInconsistentDueToUncommittedLog(DIFFSyncConsistencyTest.java:78)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
pkuwm edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731430407
@hanm @li4wang @tamaashu Yep, I also tried but since there were commits during those jUnit 5 upgrade, there are conflicts if we cherry-pick those upgrade commits. It's easier to fix it directly if we don't have more backports to branch 3.6.
I have the working code locally. @hanm if you don't have time to fix this minor issue, I could submit a PR :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731401566
@tamaashu yeah, I noticed ZOOKEEPER-3850 by itself is not sufficient. I fixed the compilation errors by applying ZOOKEEPER-3850 and some addition changes on pom.xml, but the test case failed too.
@hanm can you please fix the test failure along with the compilation error. I do agree with @tamaashu, it would be better just fix the testcases in branch 3.6 by not using JUnit5 API.
Test Case Error
=========
[INFO] Running org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 30.442 s <<< FAILURE! - in org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] testInconsistentDueToUncommittedLog Time elapsed: 30.326 s <<< FAILURE!
org.opentest4j.AssertionFailedError: waiting for server 0 being up ==> expected: <true> but was: <false>
at org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest.testInconsistentDueToUncommittedLog(DIFFSyncConsistencyTest.java:78)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731401566
@tamaashu yeah, I noticed ZOOKEEPER-3850 by itself is not sufficient. I fixed the compilation errors by applying ZOOKEEPER-3850 and some addition changes on pom.xml, but the test case failed too.
@hanm can you please fix the test failure along with the compilation error. I do agree with @tamaashu, it would be better just fix the testcases in branch 3.6 by not using JUnit5 API.
test case error
--------------
[INFO] Running org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 30.442 s <<< FAILURE! - in org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest
[ERROR] testInconsistentDueToUncommittedLog Time elapsed: 30.326 s <<< FAILURE!
org.opentest4j.AssertionFailedError: waiting for server 0 being up ==> expected: <true> but was: <false>
at org.apache.zookeeper.server.quorum.DIFFSyncConsistencyTest.testInconsistentDueToUncommittedLog(DIFFSyncConsistencyTest.java:78)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-716966318
Committers - can i get more reviews of this? I'd like to get this merged if no other issues. I hope the test case itself is straightforward to illustrate the problem and the fix itself is fairly simple (test case pass with the fix and fail w/o the fix). More discussions can be found at the ticket https://issues.apache.org/jira/browse/ZOOKEEPER-3911.
cc @eolivelli @symat @anmolnar @nkalmar @arshadmohammad
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-730946733
@hanm @li4wang I also noticed this build failure. Are you guys fixing it? If not, I think I could submit a PR for a quick fix by back porting the junit upgrade from the master branch to branch-3.6.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] li4wang edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
li4wang edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731338543
@pkuwm yes, I am fixing it. I am planning to back porting the following commits from master branch to branch-3.6 via cherry-picking.
8da9c723a ZOOKEEPER-3850: Update jUnit to 5.6 in pom.xml files
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-730869123
@li4wang thanks for reporting. I think this is because 3.6 had a different junit verison / api usage across unit tests and we didn't backup that commit to 3.6 when upgrades the tests. It should be easier to fix.
Part of the reason this was not caught during commit was because this was a cherry pick so it didn't go through the CI pipeline - also I suspect we lose our apache builds for 3.6 branches (at least my inbox was empty - otherwise the build fail notification would raise an alarm).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
pkuwm edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731430407
@hanm @li4wang @tamaashu Yep, I also tried but since there were commits during those jUnit 5 upgrade, there are conflicts if we cherry-pick those upgrade commits. It's easier to fix it directly if we don't have more backports to branch 3.6.
I have the working code locally. @hanm if you don't have time to fix this minor issue, I could submit a PR :)
A ticket is created here: https://issues.apache.org/jira/browse/ZOOKEEPER-4011
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] pkuwm edited a comment on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
pkuwm edited a comment on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-731430407
@hanm @li4wang @tamaashu Yep, I also tried but since there were commits during those jUnit 5 upgrade, there are conflicts if we cherry-pick those upgrade commits. It's easier to fix it directly if we don't have more backports to branch 3.6.
I have the work code locally. @hanm if you don't have time to fix this minor issue, I could submit a PR :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1445:
URL: https://github.com/apache/zookeeper/pull/1445#issuecomment-695747514
requesting more reviews; @lvfangmin @breed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org