lianetm commented on code in PR #15525: URL: https://github.com/apache/kafka/pull/15525#discussion_r1548547539
########## clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java: ########## @@ -1141,21 +1146,29 @@ private Map<TopicPartition, Long> beginningOrEndOffset(Collection<TopicPartition if (partitions.isEmpty()) { return Collections.emptyMap(); } + Map<TopicPartition, Long> timestampToSearch = partitions - .stream() - .collect(Collectors.toMap(Function.identity(), tp -> timestamp)); + .stream() + .collect(Collectors.toMap(Function.identity(), tp -> timestamp)); Timer timer = time.timer(timeout); ListOffsetsEvent listOffsetsEvent = new ListOffsetsEvent( - timestampToSearch, - false, - timer); - Map<TopicPartition, OffsetAndTimestamp> offsetAndTimestampMap = applicationEventHandler.addAndGet( - listOffsetsEvent, - timer); - return offsetAndTimestampMap - .entrySet() - .stream() - .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().offset())); + timestampToSearch, + timer, + false); + + Map<TopicPartition, OffsetAndTimestampInternal> offsetAndTimestampMap; + if (timeout.isZero()) { + applicationEventHandler.add(listOffsetsEvent); Review Comment: so if I get it right we are intentionally leaving this? generating an event to get offsets, when in the end we return right away without waiting for a response? I do get that the old consumer does it, and I could be missing the purpose of it, but seems to me an unneeded request, even considering the side effect of the onSuccess handler. The handler just updates the positions to reuse the offsets it just retrieved, and it does make sense to reuse the result when we do need to make a request, but I wouldn't say we need to generate an unneeded event/request just for that when the user requested offsets with max-time-to-wait=0. In any case, if we prefer to keep this, I would suggest 2 things: 1. to add a comment explaining why (handler), because it looks like a weird overhead to add the event and return, 2. to be consistent and generate the event also in the case of the `offsetsForTimes` before the early return (ln 1104). In the case of the old consumer, it's a common logic so both path, `offsetsForTimes` and `beginning/endOffsets` do the same request+return -- 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