2018-03-19 12:20 GMT+01:00 Ivan Kelly <iv...@apache.org>: > Hi folks, > > I'm currently changing some parts of pulsar to use the new APIs and > the inconsistency in the close api has raised its head again, so I'm > restarting this discussion. > > Handle has the following methods: > async: asyncClose > sync: close, getId, getLedgerMetadata > > ReadHandle has the following methods: > async: read, readUnconfirmed, readLastAddConfirmed, > tryReadLastAddConfirmed, readLastAddConfirmedAndEntry > sync: isClosed, getLength, getLastAddConfirmed > > WriteHandle has the following methods: > async: append > sync: getLastAddPushed > > Close is inconsistent with the rest of the methods for a number of reasons. > 1. No other async method uses the async* pattern. > 2. All other sync methods are querying local data and are sideeffect > free. Close can trigger I/O. > 3. Each other method has one way be being called, close has two. > > I'm not going to suggest a solution to this right now, but any > solution which gets rid of this inconsistency would be acceptable. > > New APIs shouldn't have inconsistencies like this from the outset, and > this is a blocker for me for moving the API away from the Unstable > annotations. > > What are your thoughts? >
Thank you Ivan for bringing up this discussion, I agree we have to take a decision now if we want to release the API I think that the only reason why we have a sync close() is the will to support "Closeable" interface and make it developer friendly (and findbugs/static analylis tools) and be able to use try-with-resources. If we had a CompletableFuture close() method developers would forget to wait for a result. close() in BK is an important API because it works on metadata, it not just a method which just releases resources. Is implementing Closable a "valueable" feature for us in the new API ? (I think the answer is 'yes') There was a discussion about introducing some CompletableFuture seal() method, which would be more like current close(). We could have a blocking close() which only releases resources without writes to metadata and than introduce an explicit seal() which works on metadata. Maybe we could add some "log" in case of calls to close() without seal() ? With this approach we should document very well that a seal() must be called and about the risks of not calling that seal() Enrico > > Cheers, > Ivan >
