lianetm commented on code in PR #18777:
URL: https://github.com/apache/kafka/pull/18777#discussion_r1939454766
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerMetadata.java:
##########
@@ -94,6 +95,15 @@ protected synchronized boolean retainTopic(String topic,
boolean isInternal, lon
if (isInternal && !includeInternalTopics)
return false;
+ // Keep leader metadata for topics matching the RE2J subscription.
+ // We aim to replaced this with something more efficient in
KAFKA-18117.
Review Comment:
nit: just for clarifying, even when we do KAFKA-18117 and integrate topic
IDs in the subscription state, I expect that in this case we will have no other
way but to check the assigned topics from regex to determine if we should keep
a topic metadata or not, so I don't clearly see anything "more efficient" here
directly related to that. Should we remove the comment?
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerMetadata.java:
##########
@@ -94,6 +95,15 @@ protected synchronized boolean retainTopic(String topic,
boolean isInternal, lon
if (isInternal && !includeInternalTopics)
return false;
+ // Keep leader metadata for topics matching the RE2J subscription.
+ // We aim to replaced this with something more efficient in
KAFKA-18117.
+ if (subscription.hasRe2JPatternSubscription()) {
+ for (TopicPartition topicPartition :
subscription.assignedPartitionsList()) {
+ if (topicPartition.topic().equals(topic))
+ return true;
+ }
+ }
Review Comment:
would it make sense to encapsulate this in the subscriptionState? maybe some
form of `subscription.isAssignedFromRe2J(topic)`?
In the end this check is completely based on the subscription component. And
then we would end with all regex-related check done consistently:
`return subscription.matchesSubscribedPattern(topic) ||
subscription.isAssignedFromRe2J(topic);`
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerMetadata.java:
##########
@@ -94,6 +95,15 @@ protected synchronized boolean retainTopic(String topic,
boolean isInternal, lon
if (isInternal && !includeInternalTopics)
return false;
+ // Keep leader metadata for topics matching the RE2J subscription.
+ // We aim to replaced this with something more efficient in
KAFKA-18117.
+ if (subscription.hasRe2JPatternSubscription()) {
+ for (TopicPartition topicPartition :
subscription.assignedPartitionsList()) {
+ if (topicPartition.topic().equals(topic))
+ return true;
+ }
+ }
+
Review Comment:
Effectively it would work either way I expect, but I think it's better here
just for keeping all regex-related checks in the same place.
Currently, the needsMetadata does not consider regex (even though I believe
it could, given that it's only used for retain). It's in the consumer metadata
retain logic where the java-regex is checked, so seems better to add the
re2j-regex check in the same place for consistency, it's easier to reason about
and troubleshoot.
--
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]
