squah-confluent commented on code in PR #20055:
URL: https://github.com/apache/kafka/pull/20055#discussion_r2378807309
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -3089,9 +3111,9 @@ private static boolean isNotEmpty(String value) {
* @param member The old member.
* @param updatedMember The new member.
* @param records The records accumulator.
- * @return Whether a rebalance must be triggered.
+ * @return The result of the update.
*/
- private boolean maybeUpdateRegularExpressions(
+ private UpdateRegularExpressionsResult maybeUpdateRegularExpressions(
Review Comment:
That sounds nicer in that we can't represent the fourth invalid state. I've
made the change
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -2239,21 +2239,30 @@ private
CoordinatorResult<ConsumerGroupHeartbeatResponseData, CoordinatorRecord>
// epoch 0 and that it is fully initialized.
boolean bumpGroupEpoch = group.groupEpoch() == 0;
- bumpGroupEpoch |= hasMemberSubscriptionChanged(
+ boolean subscribedTopicNamesChanged = hasMemberSubscriptionChanged(
groupId,
member,
updatedMember,
records
);
-
- bumpGroupEpoch |= maybeUpdateRegularExpressions(
+ UpdateRegularExpressionsResult updateRegularExpressionsResult =
maybeUpdateRegularExpressions(
context,
group,
member,
updatedMember,
records
);
+ // The subscription has changed when either the subscribed topic names
or subscribed topic
+ // regex has changed.
+ boolean hasSubscriptionChanged = subscribedTopicNamesChanged ||
updateRegularExpressionsResult.subscribedTopicRegexChanged;
+ // Bumping the group epoch signals that the target assignment should
be updated. We bump the
+ // group epoch when the member has changed its subscribed topic names
or the member has
+ // changed its subscribed topic regex to a regex that is already
resolved. We explicitly
+ // avoid bumping the group epoch when the new subscribed topic regex
has not been resolved
+ // yet, since we will have to update the target assignment again later.
+ bumpGroupEpoch |= subscribedTopicNamesChanged ||
updateRegularExpressionsResult.bumpGroupEpoch;
Review Comment:
It's a little tidier, yes. Updated the code.
--
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]
