Add isSealed() to ReadHandle

[sijie]

1. Please describe the feature you are requesting.

The new API is missing isClosed() on handles. It is hard to learn whether a ledger is closed or not for tailing-read applications.

2. Indicate the importance of this issue to you (blocker, must-have, should-have, nice-to-have). Are you currently using any workarounds to address this issue?

blocker

3. Provide any additional detail on your proposed use case for this feature.

N/A

[sijie]

This is required for distributedlog to implement streaming/tailing reads in apache/distributedlog#239

[ivankelly]

@sijie

1. does the WriteHandle need this?
2. Perhaps we should rename this operation? To something like isSealed(), and also change WriteHandle#close() to WriteHandle#seal, and then (as I suggested in Open should run recovery by default #774 add ReadHandle#forceSealed). Closing implies releasing of resources, while the actual operation itself is mutating state in the distributed system. I don't think it actually even releases any resources.

[ivankelly]

Updating the tutorial is blocked on this.

[sijie]

1. WriteHandle probably doesn't need this.

2. +1 for renaming it to isSealed(). I am not sure renaming WriteHandle#close() to WriteHandle#seal(), using #close() on WriteHandle has very clear definition, it doesn't actually cause confusion. sticking to #close() will gain all the syntactic sugar with AutoClosable.

[ivankelly]

Do we want all the syntactic sugar of autoclosable though? Surely sealing a ledger should be an intentional action, not a sideeffect of an exception being thrown.

[ivankelly]

Also, WriteHandle#close is inherited from Handle, so its definition isn't that clear. All handles are closable, so why do some of them mutate. I'd probably keep the close on WriteHandle, for resource cleanup, but move sealing to another method.

[sijie]

I am not sure is there a strong reason to make this change. because separating this to a method means user can close a ledger without sealing. I would suggest we should have one simple close for WriteHandle. regarding javadoc, we can update the javadoc for #close() to be more explicitly.

[ivankelly]

I'm ok with keeping just #close/asyncClose, but in that case, close should only be part of WriteHandle. The reason I suggested keeping #close and #seal is in case we ever do need to free resources in all handles, we already have #close being called, but maybe that doesn't make sense.

[ivankelly]

@sijie @eolivelli

Also, asyncClose and close in Handle are a completely different pattern to the rest of the calls. There's no asyncX and X for everything else. They just return a future. #close/#seal should follow the same pattern. It seems like we jump through a lot of hoops just to get AutoCloseable, when the value of AutoCloseable for this is questionable.

I think part of the motivation for have a close is that things are called Handle, and this is suggesting that LedgerHandle/WriteHandle/ReadHandle actually hold on to resources somewhere else (like how a FileHandle holds a file descriptor), and it also suggests that when we are finished with *Handle the resource needs to be freed. However, this is not the case for any of the Handle types. They just bundle a bunch of integers together to allow us to read/write. They're contexts more than anything.

[sijie]

@ivankelly ReadOnlyLedgerHandle need to release listener registered at ledger manager. so we need close at ReadHandle.

I am not sure what you are trying to propose in the last comment. Are you suggesting renaming the API from Handle to something else?

[ivankelly]

@sijie If we need #close on all Handle, then there's no need for rename, but I was going to propose renaming WriteHandle -> LedgerWriter and ReadHandle ->LedgerReader.

There's still in inconsistency of #asyncClose and all other methods. I think i'd prefer to remove asyncClose, leave close as autoclosable, and add a method to WriteHandle.

CompletableFuture<Void> seal();

Especially if were going to rename isClosed to isSealed.

I don't think there's a problem of people calling #close without sealing. If it happens, the writer will start writing to the next ledger, but tailers will block waiting for the previous ledger to be sealed. This should be fairly obvious, and quickly fixed. In any case, there's no risk of data corruption.

[sijie]

okay I will change isSealed back to isClosed.

I am not sure we want to do this rename here. I don't see it is necessary. but if you do have a strong feeling, please make a BP and bring it to mailing list. because current set of API was proposed by Enrico and reviewed before. so if we are going to make the rename change, we need another BP to review it.

regarding seal and close, my take is - this is fairly clear to people. I don't think there is a real need to split this into two methods of seal() and close() -- to me it is complicating the usage of WriteHandle. However @eolivelli might have a different opinion, since he is the original interface proposer.

[ivankelly]

I'll create a BP. There's a couple of issues with API that should be addressed.

…

On Wed, Nov 29, 2017 at 11:29 AM, Sijie Guo ***@***.***> wrote: okay I will change isSealed back to isClosed. I am not sure we want to do this rename here. I don't see it is necessary. but if you do have a strong feeling, please make a BP and bring it to mailing list. because current set of API was proposed by Enrico and reviewed before. so if we are going to make the rename change, we need another BP to review it. regarding seal and close, my take is - this is fairly clear to people. I don't think there is a real need to split this into two methods of seal() and close() -- to me it is complicating the usage of WriteHandle. However @eolivelli <https://github.com/eolivelli> might have a different opinion, since he is the original interface proposer. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWq5wRTyNJKEm_CAS6w5hLFTuA9gAEks5s7UAWgaJpZM4QoNlw> .

[eolivelli]

@sijie @ivankelly
I think that renaming asyncClose() to seal() is not a good option
In fact calling asyncClose on a readonly handle does not 'seal' it, it works only for writers.
It is clear for a BK user that a ledger can be written only once, so it is an expected behavior that WriteHandle.close() closes it and there is no other way to append other data.

For "isClosed()", as an user I would expect that Object.isClosed() will return true if I have called Object.close() not if someone else (from another application/JVM/network...) have "sealed" the ledger, so that no new entry is expected.
So I think it is a good idea to think about this API now.
I am in favour of isSealed() or something that explains that no other entry will be added in the future

[ivankelly]

I think that renaming asyncClose() to seal() is not a good option
In fact calling asyncClose on a readonly handle does not 'seal' it, it works only for writers.
asyncClose is meaningless for anything but writers. closing is releasing local resources, so there's no I/O that would require anything async.

Anyhow, I'll create a BP.

[ivankelly]

@sijie @eolivelli could you check #760 again, so I can create the BP using the new workflow.