lianetm commented on code in PR #15585:
URL: https://github.com/apache/kafka/pull/15585#discussion_r1539955531
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/HeartbeatRequestManager.java:
##########
@@ -550,6 +550,11 @@ public ConsumerGroupHeartbeatRequestData
buildRequestData() {
} else {
// SubscribedTopicRegex - only sent if has changed since the
last heartbeat
// - not supported yet
+ TreeSet<String> subscribedTopicNames = new
TreeSet<>(this.subscriptions.subscription());
+ if (sendAllFields ||
!subscribedTopicNames.equals(sentFields.subscribedTopicNames)) {
+ data.setSubscribedTopicNames(new
ArrayList<>(this.subscriptions.subscription()));
+ sentFields.subscribedTopicNames = subscribedTopicNames;
+ }
Review Comment:
I guess the if/else was initially added to set the structure that there will
be 2 paths:
1- send the broker the explicit topic names to subscribe to (this includes
subscribe with topic list and with Pattern, that the client resolves to a topic
list)
2- send the broker a regex, for the broker to resolve it (the new
SubscriptionPattern)
So, key point is that, with the concepts above, up to this PR (included),
we're still only supporting path 1, so that' why I would say we better get rid
of the if/else, and include it only when we do implement the 2nd path, and we
properly think about the condition to use, and add a body to the else
--
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
