tombentley commented on a change in pull request #9878:
URL: https://github.com/apache/kafka/pull/9878#discussion_r662933606
##########
File path:
clients/src/main/java/org/apache/kafka/common/internals/KafkaFutureImpl.java
##########
@@ -247,17 +138,32 @@ public boolean completeExceptionally(Throwable
newException) {
*/
@Override
public synchronized boolean cancel(boolean mayInterruptIfRunning) {
- return completeExceptionally(new CancellationException()) || exception
instanceof CancellationException;
+ return completableFuture.cancel(mayInterruptIfRunning);
+ }
+
+ // We need to deal with differences between KF's historic API and the API
of CF:
+ // CF#get() does not wrap CancellationException in ExecutionException (nor
does KF).
+ // CF#get() always wraps the _cause_ of a CompletionException in
ExecutionException (which KF does not).
+ //
+ // The semantics for KafkaFuture are that all exceptional completions of
the future (via #completeExceptionally() or exceptions from dependants)
+ // manifest as ExecutionException, as observed via both get() and getNow().
+ private void maybeRewrapAndThrow(Throwable cause) {
+ if (cause instanceof CancellationException) {
+ throw (CancellationException) cause;
+ }
}
/**
* Waits if necessary for this future to complete, and then returns its
result.
*/
@Override
public T get() throws InterruptedException, ExecutionException {
- SingleWaiter<T> waiter = new SingleWaiter<>();
- addWaiter(waiter);
- return waiter.await();
+ try {
+ return completableFuture.get();
+ } catch (ExecutionException e) {
+ maybeRewrapAndThrow(e.getCause());
+ throw e;
Review comment:
It doesn't seem to help with other places where we catch and rethrow
because they catch unchecked exceptions `Error` and `RuntimeException` whose
only supertype is `Throwable`, so we end up in a painful place due to Java's
lack of abstraction for "unchecked exception".
For the existing call sites the `void` return type you proposed doesn't work
with javac's definite return analysis so I have to declare `private <T> T
rewrapAndThrow(CompletionException exception)` and `private <T> T
rewrapAndThrow(ExecutionException exception)`. The method always throws, so the
return type of `T` is just there to appease `javac` via a `return
rewrapAndThrow(...)` at the call site.
I can do this is you want, but I feel it's making the code less clear. The
`rewrapAndThrow(CompletionException exception)` only has a single caller, so
it's of little benefit.
I guess the other alternative would be to inline the method and just comment
each site as to what's going on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
