Add sync variants of all methods in handles #1288
Conversation
As discussed on the mailing list [1], this patch removes the inconsistency around the naming of the close call on the new handle APIs, by creating sync versions of each async calls, and renaming the async versions to have the suffix "Async". Most of the changes are very mechanical - just a copy of the old method and some small fixups the javadoc. One thing to note is that I've made a copy of the close and closeAsync methods in the WriteHandle interface, so that the ReadHandle and Handle javadoc for these methods do not have to talk about what it means to close/seal a ledger. Another change is that I've removed the SneakyThrows from close, that would have also been needed on the other sync methods. Instead, I pass a exception handler to FutureUtils which generates a BKException. [1] https://lists.apache.org/thread.html/c3784cffb949438510d21e5eac8c0351865c6748c42c380e673a60db@%3Cdev.bookkeeper.apache.org%3E
| @@ -21,7 +21,7 @@ | |||
| package org.apache.bookkeeper.client.api; | |||
|
|
|||
| import java.util.concurrent.CompletableFuture; | |||
| import lombok.SneakyThrows; | |||
| import org.apache.bookkeeper.client.impl.BKExceptionHandler; | |||
There was a problem hiding this comment.
Make BKExceptionHandler.HANDLER as part of BKException. rather than having a separate class. this also prevent api package referencing impl package, which allows producing a clean api package.
|
@sijie updated |
|
@ivankelly you need to fix some tests. |
|
@ivankelly this is an important change to the public API. But shouldn't it need some kind of discussion/vote on the mailing list ? Cc @sijie |
|
retest this please |
|
Sorry, I was not clear. The discussion happened but not in the usual form during last period (like BPs...) and no formal vote thread has been sent. |
|
@eolivelli - I think this is not proposing a brand new API. The original new API was introduced in a BP, this change here is more a incremental change to fix the inconsistency in the new API. so I don't think it worth a BP. but if you really feel strong about it, we can do a BP. |
|
Makes sense to me I feel fine with merging this PR and move forward. |
|
I did not want to close this one |
|
retest this please |
|
@eolivelli there was a integration test failure, so kicking again. I think it's unrelated, but want to be sure. Also, too late in the day to dig deeper :) |
As discussed on the mailing list [1], this patch removes the
inconsistency around the naming of the close call on the new handle
APIs, by creating sync versions of each async calls, and renaming the
async versions to have the suffix "Async".
Most of the changes are very mechanical - just a copy of the old
method and some small fixups the javadoc. One thing to note is that
I've made a copy of the close and closeAsync methods in the
WriteHandle interface, so that the ReadHandle and Handle javadoc for
these methods do not have to talk about what it means to close/seal a
ledger.
Another change is that I've removed the SneakyThrows from close, that
would have also been needed on the other sync methods. Instead, I pass a
exception handler to FutureUtils which generates a BKException.
[1] https://lists.apache.org/thread.html/c3784cffb949438510d21e5eac8c0351865c6748c42c380e673a60db@%3Cdev.bookkeeper.apache.org%3E