jonmv commented on code in PR #1925:
URL: https://github.com/apache/zookeeper/pull/1925#discussion_r997117455


##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java:
##########
@@ -543,35 +543,153 @@ protected long registerWithLeader(int pktType) throws 
IOException {
      * @throws InterruptedException
      */
     protected void syncWithLeader(long newLeaderZxid) throws Exception {
-        QuorumPacket ack = new QuorumPacket(Leader.ACK, 0, null, null);
-        QuorumPacket qp = new QuorumPacket();
         long newEpoch = ZxidUtils.getEpochFromZxid(newLeaderZxid);
-
         QuorumVerifier newLeaderQV = null;
 
-        // In the DIFF case we don't need to do a snapshot because the 
transactions will sync on top of any existing snapshot
-        // For SNAP and TRUNC the snapshot is needed to save that history
-        boolean snapshotNeeded = true;
-        boolean syncSnapshot = false;
+        class SyncHelper {
+
+            // In the DIFF case we don't need to do a snapshot because the 
transactions will sync on top of any existing snapshot.
+            // For SNAP and TRUNC the snapshot is needed to save that history.
+            boolean willSnapshot = true;
+            boolean syncSnapshot = false;
+
+            // PROPOSALs received during sync, for matching up with COMMITs.
+            Deque<PacketInFlight> proposals = new ArrayDeque<>();
+
+            // PROPOSALs we delay forwarding to the ZK server until sync is 
done.
+            Deque<PacketInFlight> delayedProposals = new ArrayDeque<>();
+
+            // COMMITs we delay forwarding to the ZK server until sync is done.
+            Deque<Long> delayedCommits = new ArrayDeque<>();

Review Comment:
   So I'm not sure we really need to delay them in here, now that the 
`startUpWithoutServing()` exists. Perhaps we could just forward these to the ZK 
server immediately, instead of this double bookkeeping? 
   I suspect the out-of-order ACKs made that a non-option previously, but with 
the ability to delay ACKs, in the SyncRequestProcessor, added previously in 
this PR, that would no longer be a problem. WDYT? 



-- 
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.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to