kirktrue commented on code in PR #17700:
URL: https://github.com/apache/kafka/pull/17700#discussion_r1915522609
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractFetch.java:
##########
@@ -408,7 +388,21 @@ protected Map<Node, FetchSessionHandler.FetchRequestData>
prepareFetchRequests()
long currentTimeMs = time.milliseconds();
Map<String, Uuid> topicIds = metadata.topicIds();
- for (TopicPartition partition : fetchablePartitions()) {
+ // This is the set of partitions that have buffered data
+ Set<TopicPartition> buffered =
Collections.unmodifiableSet(fetchBuffer.bufferedPartitions());
+
+ // This is the set of partitions that do not have buffered data
+ Set<TopicPartition> unbuffered =
Set.copyOf(subscriptions.fetchablePartitions(tp -> !buffered.contains(tp)));
+
+ if (unbuffered.isEmpty()) {
+ // If there are no partitions that don't already have data locally
buffered, there's no need to issue
+ // any fetch requests at the present time.
+ return Collections.emptyMap();
+ }
+
+ Set<Integer> nodesWithBufferedPartitions =
nodesWithBufferedPartitions(buffered, currentTimeMs);
Review Comment:
> Have we seen an impact on consumer CPU?
Not based on this change, no.
> also, i was wondering if getting nodes for buffered partitions was
something we can pre-process while updating fetchBuffer. but not sure that
would actually improve performance. I guess it would still help since we
process it once
That's an interesting idea. I believe we have the `Node` information on hand
when the fetch buffer entry is created, so it's probably doable without a ton
of work. I guess the challenge would be keeping the leader/follower information
up-to-date. Because that requires some exploration, do you mind if we slice
that off into a separate Jira?
Thanks!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
