jeffrey-xiao commented on code in PR #1925:
URL: https://github.com/apache/zookeeper/pull/1925#discussion_r1095119672
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java:
##########
@@ -190,23 +190,23 @@ public long getServerId() {
}
@Override
- public synchronized void shutdown() {
+ public synchronized void shutdown(boolean fullyShutDown) {
if (!canShutdown()) {
+ super.shutdown(fullyShutDown);
Review Comment:
Should this line be here? It seems like we're calling
`super.shutdown(fullyShutDown)` twice.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/DIFFSyncConsistencyTest.java:
##########
@@ -49,7 +49,7 @@ public class DIFFSyncConsistencyTest extends
QuorumPeerTestBase {
private MainThread[] mt = new MainThread[SERVER_COUNT];
@Test
- @Timeout(value = 120)
+ @Timeout(value = 20)
Review Comment:
What's the reason for this timeout change? The tests seems to pass through
this change.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java:
##########
@@ -543,35 +543,150 @@ 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<>();
+
+ public void syncSnapshot() {
+ syncSnapshot = true;
+ }
+
+ public void noSnapshot() {
+ willSnapshot = false;
+ }
+
+ public void propose(PacketInFlight pif) {
+ proposals.add(pif);
+ delayedProposals.add(pif);
+ }
+
+ public PacketInFlight nextProposal() {
+ return proposals.peekFirst();
+ }
+
+ public void commit() {
+ PacketInFlight packet = proposals.remove();
+ if (willSnapshot) {
+ zk.processTxn(packet.hdr, packet.rec);
+ delayedProposals.remove();
+ } else {
+ delayedCommits.add(packet.hdr.getZxid());
+ }
+ }
+
+ public void proposeAndCommit(PacketInFlight packet) {
+ // Should be able to do propose(packet); commit(); because
INFORM with non-empty proposals would be an error ...
Review Comment:
What's the reason for not doing `propose(packet)` followed by `commit()`?
Seems like it's equivalent to this block.
--
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: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
