Github user revans2 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/180#discussion_r102245094
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -303,15 +305,38 @@ public void loadData() throws IOException,
InterruptedException {
public void takeSnapshot(){
--- End diff --
@eribeiro An AtomicBoolean is not enough as the SyncRequestProcessor
ignores it, only the JMX and admin commands use it.
@flier there are several things going on here and I don't know the
reasoning for all of them but I can guess.
1. Only take one snapshot at a time. I don't think this is super
critical, because it is not a correctness problem. Having multiple snapshots
happening at the same time should work correctly even in the face of crashes,
but it becomes a performance problem. There is a limited amount of CPU,
Memory, and most importantly disk bandwidth and IOps. The last successful
snapshot wins. So having multiple snapshots all going at the same time means
we are likely to be doing a lot of wasted work if everything does happen
successfully. If we can schedule the snapshots so there is only one going on
at a time then the full bandwidth and IOps can go to that snapshot. Even
better would be to space them out, so if we force a snapshot it makes
SyncRequestProcessor reset its counter so we don't take another one until X
more transactions have been processed.
2. Taking a snapshot at the wrong time and then crashing can corrupt the DB
on that node. This is potentially critical. The probability of this happening
is very very small, but if we can fix it so it can never happen, I would be
much happier.
I think we can fix both issues at the same time, by making JMX and Admin
use SyncRequestProcessor instead of going to the ZookeeperServer directly. If
no SyncRequestProcessor is ready then we can return such to the user so they
know the node is not ready to take a snapshot.
I also really would like to understand the use case for this command,
because I just don't see much value to a user to force another snapshot if one
is already in progress. I also would like to understand when you would want to
take a snapshot and if these changes would too severely limit that.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
