tombentley commented on a change in pull request #9878:
URL: https://github.com/apache/kafka/pull/9878#discussion_r663955317
##########
File path:
clients/src/main/java/org/apache/kafka/common/internals/KafkaFutureImpl.java
##########
@@ -267,50 +180,82 @@ public T get() throws InterruptedException,
ExecutionException {
@Override
public T get(long timeout, TimeUnit unit) throws InterruptedException,
ExecutionException,
TimeoutException {
- SingleWaiter<T> waiter = new SingleWaiter<>();
- addWaiter(waiter);
- return waiter.await(timeout, unit);
+ try {
+ return completableFuture.get(timeout, unit);
+ } catch (ExecutionException e) {
+ maybeThrowCancellationException(e.getCause());
+ throw e;
+ }
}
/**
* Returns the result value (or throws any encountered exception) if
completed, else returns
* the given valueIfAbsent.
*/
@Override
- public synchronized T getNow(T valueIfAbsent) throws InterruptedException,
ExecutionException {
- if (exception != null)
- wrapAndThrow(exception);
- if (done)
- return value;
- return valueIfAbsent;
+ public synchronized T getNow(T valueIfAbsent) throws ExecutionException {
Review comment:
I wouldn't call it `get` because it has different semantics to the
existing get methods, so I think it should have a distinct name. And I can't
think of a succinct alternative to `getNow`. Something like `getOrDefault`
doesn't convey they key difference from `get` (the lack of blocking).
`getImmediately` is just a bit verbose.
The API of KafkaFuture has always been a subset of `CompletableFuture`.
Introducing a new method would break that. While it would be discoverable while
the deprecated method remained to point the way it wouldn't be as discoverable
once the old `getNow` was removed.
All deprecation really does is let the programmer defer fixing the problem.
But it's most likely not a huge problem to fix (i.e. they've likely caught the
exception close to the method call, rather than defining many layers of API
with `throws InterruptedException`).
A new method also means their code is then not binary compatible with older
client versions.
So on balance I personally favour just removing the `throws
InterruptedException` in 3.0 and letting the programmer clean up the code
sooner rather than later.
There is always the "do nothing" alternative.
--
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
