[GitHub] [zookeeper] hanm commented on a change in pull request #1445: ZOOKEEPER-3911: Data inconsistency caused by DIFF sync uncommitted log

[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