search

zk1931 / jzab

ZooKeeper Atomic Broadcast in Java
http://zk1931.github.io/jzab/master/
Apache License 2.0
54 stars 23 forks source link

Let user decides when to take snapshot #190

Closed EasonLiao closed 10 years ago

EasonLiao commented 10 years ago

Like goraft, let user decides when to take snapshot, user just call Zab.takeSnapshot and Zab is responsible calling save callback. User just decides the timing of taking snapshot, Zab still manages the snapshot file for them.

The benefit is there's more flexibility for application, some application might have specific preference of when to take snapshot.

ghost commented 10 years ago

sounds good. what's the signature of the takesnapshot method? it needs to notify the caller when snapshot completes, either though a callback or by returning a future and letting the caller add listeners.

EasonLiao commented 10 years ago

I feel we don't need future and callbacks. Since we'll take snapshot via callbacks, so user will know when snapshot gets complete.

ghost commented 10 years ago

ok that's a fair point. how do you communicate errors back to the caller? how about snapshot file names? maybe the caller wants to know where the snapshot has been saved.

EasonLiao commented 10 years ago

My understanding is that user shouldn't care about where the file is stored, Jzab will manage the files for user. Or we can make the takeSnapshot blocking until the snapshot is completed, like goraft. What do you think?

ghost commented 10 years ago

i think you know what i'm going to say, but no, we shouldn't make it blocking.

the caller might want to know what the snapshot filename is, maybe to make a backup or something. anyways, the key point i want to make is that the snapshot call should be non-blocking, and there should be a way to communicate the result back to the caller.

EasonLiao commented 10 years ago

how about adding another callback like snapshotSaved and pass the file name and exception back to application?

ghost commented 10 years ago

how do we handle Zab.remove()? does the caller get notified when it gets removed from the cluster?

EasonLiao commented 10 years ago

Yes, before it gets shut down it will get notified.

EasonLiao commented 10 years ago

@m1ch1

ghost commented 10 years ago

is this patch ready to be checked in?

one request: i prefer having a single commit per pull request unless commits are trivial. otherwise i end up reviewing all the commits multiple times until they get merged even when some of them are not updated.

EasonLiao commented 10 years ago

you can check in this patch, I guess the associating the context object with the request I'll address all of them in another commit.

Sorry for the multiple commits per PR problem, I'll do it in the way you suggested.