Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/112#discussion_r121793647
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Learner.java ---
@@ -390,6 +391,7 @@ else if (qp.getType() == Leader.SNAP) {
+ Long.toHexString(qp.getZxid()));
System.exit(13);
}
+ zk.getZKDatabase().setlastProcessedZxid(qp.getZxid());
--- End diff --
The fix looks good to me.
I think we should also set the zxid extracted from the current proposal
packet after each proposal is
[committed](https://github.com/arshadmohammad/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/quorum/Learner.java#L460).
Otherwise the follower will have a lagged view of the committed transactions,
because with the fix in this patch, we will never do setlastProcessedZxid
during a DIFF sync. For example imagine a case like this:
* Follower has its latest zxid with value a before DIFF SYNC happens.
* Leader send over proposals with zxids value b, c, d.
* Follower received and applied proposals b and c. Before follower had a
chance to get hands on d, network partition happens.
* Now partition healed, follower will do a DIFF think again. Because the zk
database would not be reloaded from logs (it's already initialized), follower
has a skewed view of the world - it thinks it only has tnx a, but in fact it
has a, b, and c. So rather asking b, c, and d, the follower could just ask d.
Anyway I think it is an optimization that might worth doing - it is not
functional critical because the idempotent nature of applying transactions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
