cadonna commented on PR #15640: URL: https://github.com/apache/kafka/pull/15640#issuecomment-2090517636
> > > > Here I have a comment, I could not put at the right location in the code: > > > > On line 1362, in commitSync() the consumer waits on the commitFuture with a timer. I think, it should not wait on a timer there since we already wait on a timer in the background thread. > > > > > > > > > I agree. What about the timed wait in awaitPendingAsyncCommitsAndExecuteCommitCallbacks()? > > > > > > Agree we should not wait on the `commitFuture` with a timer because the deadline is contained in the event we submitted, and already enforced by the reaper, and not clear about what the proposed relationship with `awaitPendingAsyncCommitsAndExecuteCommitCallbacks` is?? > > I would expect we only need to call `ConsumerUtils.getResult(commitFuture);`, and that is consistent with how we get results for all other completable events now: > > > > * we create an event with a deadline > > * we call `applicationEventHandler.addAndGet(event)` > > > > For the commit case that flow has a different shape just because we use `applicationEventHandler.add(event)` [here](https://github.com/apache/kafka/blob/097522abd6b51bca2407ea0de7009ed6a2d970b4/clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java#L775), to cater for commit sync and async, but we should still apply the same approach and just call get without any time boundary I would say. > > Here's my reasoning on the need for a `Timer`-based `get()` in `awaitPendingAsyncCommitsAndExecuteCommitCallbacks()`... > > The `Future` that's referenced in `lastPendingAsyncCommit` comes from an `AsyncCommitEvent` and has a hard-coded deadline of `Long.MAX_VALUE`. As such, the `CompetableEventReaper` in the network thread will never prune that event. Without a timeout when calling `get()` on the `lastPendingAsyncCommit`, the caller could hang for up to `request.timeout.ms` while we wait for the network I/O request to complete (or timeout). > > @cadonna @lianetm @lucasbru—does that make sense? CMIIW, please 🙏 I think, we are talking about two separate things here: 1. change `ConsumerUtils.getResult(commitFuture, requestTimer);` to `ConsumerUtils.getResult(commitFuture);` 2. Do we need a timer for `awaitPendingAsyncCommitsAndExecuteCommitCallbacks(requestTimer, true);` As far as I understand, we agree on 1 but do not know if there is a better solution for 2. Is this correct? Currently, I do not see a better way than using a timer on awaiting the execution of the async commit callback. As @kirktrue pointed out, since the async commit does basically not have a timeout, we cannot wait on the deadline of the `AsyncCommitEvent`. We can also not wait for the deadline of the `SyncCommitEvent` since if that event completes before the timeout, we would not wait enough for the completion of the async commit callback. -- 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
