lianetm commented on code in PR #15585:
URL: https://github.com/apache/kafka/pull/15585#discussion_r1538173798
##########
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:
With this addition we end up with the exact same code repeated for the `if`
and `else`, so I would say we should find a better way of doing this. First
solution that comes to mind is to remove the if/else. In the end, we have a
single case to handle here: send explicit subscriptions (topic names) to the
broker (from the HB Mgr POV and to the broker, it does not make a diff if the
topic list came from a call to subscribe with topics or a call to subscribe
with Pattern that we internally resolved on the client)
When we take on the next task of supporting the new regex, we'll actually
have to send something different here, so we can decide then how to best
differentiate the 2 cases. For now, including this PR, we only support 1 case
regarding the content of what we send in the HB regarding subscription. Makes
sense?
--
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]
