Re: [DISCUSS] Inconsistency in Handle based APIs - Specifically "close"

[Enrico Olivelli]

+1 on 3 forse me too

Il mar 20 mar 2018, 09:15 Sijie Guo  ha scritto:

> On Tue, Mar 20, 2018 at 12:50 AM, Ivan Kelly  wrote:
>
> > On Mon, Mar 19, 2018 at 6:37 PM, Sijie Guo  wrote:
> > > It is not a blocker for me.
> > >
> > > But if we want consistency, either applying pattern "asyncXYZ()" or
> > > "xyzAsync()" for async operations works for me.
> > xyzAsync is better than asyncXyz, as it will put the async and sync
> > versions together in the javadoc.
> >
> > So, there's 3 options here.
> >
> > 1. Remove Closeable
> > 2. Some kind of split of sealing and close
> > 3. Create sync and async versions of all with Async suffix.
> >
> > I think 3 is the most palatable. If there's no objections I'll push a
> > patch later today.
> >
>
> +1 on 3
>
> >
> > -Ivan
> >
>
-- 


-- Enrico Olivelli

Re: [DISCUSS] Inconsistency in Handle based APIs - Specifically "close"

[Sijie Guo]

On Tue, Mar 20, 2018 at 12:50 AM, Ivan Kelly  wrote:

> On Mon, Mar 19, 2018 at 6:37 PM, Sijie Guo  wrote:
> > It is not a blocker for me.
> >
> > But if we want consistency, either applying pattern "asyncXYZ()" or
> > "xyzAsync()" for async operations works for me.
> xyzAsync is better than asyncXyz, as it will put the async and sync
> versions together in the javadoc.
>
> So, there's 3 options here.
>
> 1. Remove Closeable
> 2. Some kind of split of sealing and close
> 3. Create sync and async versions of all with Async suffix.
>
> I think 3 is the most palatable. If there's no objections I'll push a
> patch later today.
>

+1 on 3

>
> -Ivan
>

Re: [DISCUSS] Inconsistency in Handle based APIs - Specifically "close"

[Ivan Kelly]

On Mon, Mar 19, 2018 at 6:37 PM, Sijie Guo  wrote:
> It is not a blocker for me.
>
> But if we want consistency, either applying pattern "asyncXYZ()" or
> "xyzAsync()" for async operations works for me.
xyzAsync is better than asyncXyz, as it will put the async and sync
versions together in the javadoc.

So, there's 3 options here.

1. Remove Closeable
2. Some kind of split of sealing and close
3. Create sync and async versions of all with Async suffix.

I think 3 is the most palatable. If there's no objections I'll push a
patch later today.

-Ivan

Re: [DISCUSS] Inconsistency in Handle based APIs - Specifically "close"

[Sijie Guo]

It is not a blocker for me.

But if we want consistency, either applying pattern "asyncXYZ()" or
"xyzAsync()" for async operations works for me.

- Sijie


On Mon, Mar 19, 2018 at 4:20 AM, Ivan Kelly  wrote:

> 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?
>
> Cheers,
> Ivan
>

Re: [DISCUSS] Inconsistency in Handle based APIs - Specifically "close"

[Ivan Kelly]

> Is implementing Closable a "valueable" feature for us in the new API ?  (I
> think the answer is 'yes')

I'm not so sure how useful Closeable is here. It is handy in tests,
but in production code you are never going to use the
try-with-resources pattern, as you'll be using async calls for
everything else. I think people not waiting on a returned
CompletableFuture is a bigger issue, but maybe even that isn't
important (see below).

> There was a discussion about introducing some CompletableFuture seal()
> method, which would be more like current close().

Yes, there was a BP which just ended going around in circles and I got
frustrated and closed it. Part of the problem was there were many
things being discusses at the same time, so now I just want to
concentrate on the close inconsistency.

> With this approach we should document very well that a seal() must be
> called and about the risks of not calling that seal()

What are those risks? If I never called close() or seal() on my
WriteHandle, what is the worst thing that could happen? I don't think
much bad could happen at all, because not calling close or seal is the
same as crashing before you do. So not calling it means that the next
person to open the ledger has to deal with it, which is a latency hit
for them. Even in the case where you are rolling your ledger, and then
continue writing to a new one, I don't think there's a problem. If you
don't close, then any entries that have been successfully written will
continue to be successfully written, so whether you record this in the
metadata isn't important. And you don't need to fence, because in this
case you are the writer.

Anyhow, I'm not arguing for a particular solution here, just
highlighting that the operation of "close" isn't as vital as it has
always appeared to be.

Cheers,
Ivan

Re: [DISCUSS] Inconsistency in Handle based APIs - Specifically "close"

[Enrico Olivelli]

2018-03-19 12:20 GMT+01:00 Ivan Kelly :

> 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
>