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
