Hi, I would prefer if the "checkpoint" method is renamed to "tryCheckpoint" or similar, if it is a "best effort" method (which seems to be the case, as it uses commitSemaphore.tryAcquire()). If a checkpoint can't be created, then I would return null. This is the same as FileChannel.tryLock (returns null if the lock can't be acquired). >From a user point of view, it would be strange if FileChannel.tryLock returns a "special case" non-null value, or returns a lock that can't be released...
Regards, Thomas On 25/08/14 10:17, "Marcel Reutegger" <[email protected]> wrote: >Hi, > >On 22/08/14 16:31, "Alex Parvulescu" <[email protected]> wrote: >>Following OAK-2039 there was a discussion around the current design of >>the >>#checkpoint apis. [0] >> >>It looks a bit confusing that you can call the apis to create a >>checkpoint >>and get back a reference but when retrieving it, it might not exist, even >>if the calls are back to back. >>With OAK-2039 I've added some warning logs when a checkpoint cannot be >>created but a ref is still returned, to understand if this is a system >>load >>problem, or something more profound. > >what is the reason the SegmentNodeStore does a >commitSemaphore.tryAcquire() >instead of a commitSemaphore.acquire() like in SegmentNodeStore.merge()? > >>I believe that nobody has any issues with the #retrieve method, all the >>confusion is really about the #checkpoint parts, currently marked as >>'@Nonnull'. >> >>Alternatives mentioned are >> - return null if the checkpoint was not created >> - throw en exception >> >>I vote -0 for the change, I believe that making this more complicated >>than >>it needs to be (more null checks, or a try/catch) doesn't really benefit >>anybody. > >I think we should improve it somehow because I find the current behaviour >quite confusing. The current implementation of >SegmentNodeStore.checkpoint() >IMO violates the contract. It may return a string reference to a >checkpoint >which was never created and obviously won't be valid for the requested >lifetime. > >In my view, a client should be able to detect this in a simple way. Right >now you would have to call retrieve() to find out if checkpoint() actually >worked. > >Returning a null value works better if we specify under what conditions >no checkpoint can be created. After all a client would have to implement >some code in response to a null value. E.g. should it retry later, because >the checkpoint cannot be created when the system is under load? This would >be a good fit if we keep the current implementation in SegmentNodeStore. > >An exception works better if we say an implementation should always be >able to create a checkpoint and only fail if it cannot perform the >operation because of e.g. an underlying IOException. > >Regards > Marcel >
