ZOOKEEPER-4766: Avoid unnecessary snapshots during leader election #2153
Conversation
| @@ -546,7 +554,9 @@ public void loadData() throws IOException, InterruptedException { | |||
| .forEach(session -> killSession(session, zkDb.getDataTreeLastProcessedZxid())); | |||
|
|
|||
| // Make a clean snapshot | |||
There was a problem hiding this comment.
Please update the comment in what cases needSnapshot would be true/false and why.
There was a problem hiding this comment.
Sounds good, added a comment 👍
|
Hi @tisonkun, thank you for taking a look! I have added an explanatory comment. Please let me know if this seems good to you. Also, @horizonzy mentioned a concern regarding the current session in the original PR #2085. Does anything need to be done to address this concern? I am not sure I fully understand that scenario. |
|
Hi @tisonkun @horizonzy @eolivelli, thank you all for taking a look so far! Just wanted to check in to see if there are any other changes needed here or if the concern regarding the current session needs to be addressed in some way |
| /* A snapshot is not needed when loading data during leader election. A new snapshot is not needed to return to | ||
| * the current state because the current state was reached by loading the data tree using an old snapshot. | ||
| * During leader election, there are no ongoing transactions that could be lost either. If a snapshot is needed | ||
| * to send to a learner during leader election, that snapshot will be taken as part of leader election, so | ||
| * snapshotting does not need to be done here as well. | ||
| */ |
There was a problem hiding this comment.
I believe this should answer your question, right @horizonzy ?
There was a problem hiding this comment.
zookeeper's snapshot not only persists the dataTree but also persists the session and session expiration time, I'm concerned that if we don't take a snapshot here, the session data may not be accurate during the next restart.
@eolivelli /cc
There was a problem hiding this comment.
Thanks a lot for the info @horizonzy , is this session info used for something upon restart that is necessary for correctness? Is it mainly important for telemetry? If it is not needed for correctness could skipping this snapshotting step during leader election be made a configurable option?
|
Hi @tisonkun @eolivelli @horizonzy, just wanted to check in to see if you have gotten a chance to take another look at this |
No description provided.