tombentley commented on a change in pull request #9878: URL: https://github.com/apache/kafka/pull/9878#discussion_r662990335
########## 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: There don't seem to be any good options here. We could throw from a single method if you're willing to have the method like this ```java private <T, E extends Exception> T throwUnwrappingCancellationException(E exception) throws E { if (exception.getCause() instanceof CancellationException) { throw (CancellationException) exception.getCause(); } throw exception; } ``` and the call sites to look like this: ```java return throwUnwrappingCancellationException(e); ``` The `return <methodThatAlwaysThrows>` is nasty, and I can't think of a good method name. Personally I don't think this is better than naming the current method `maybeThrowCancellationException` as you suggest, and living with the throw in two places, so I've gone with that. -- 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