Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
kirktrue commented on PR #21013: URL: https://github.com/apache/kafka/pull/21013#issuecomment-3609556061 @AntonVasant—for posterity, can you update the PR title to match the correct 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
kirktrue commented on PR #21013: URL: https://github.com/apache/kafka/pull/21013#issuecomment-3609528286 Ah, I think this is KAFKA-19899. -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
kirktrue commented on PR #21013: URL: https://github.com/apache/kafka/pull/21013#issuecomment-3609526291 The PR title lists KAFKA-19891, but that Jira is related to logging, which is unrelated to the changes here 🤔 -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
dajac commented on PR #21013: URL: https://github.com/apache/kafka/pull/21013#issuecomment-3608182500 I could not cherry-pick it to 4.1 because the code is different in this area. @AntonVasant Would you be willing to raise a PR for the 4.1 branch? -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
dajac commented on PR #21013: URL: https://github.com/apache/kafka/pull/21013#issuecomment-3608020134 Merged to trunk and 4.2. -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
dajac merged PR #21013: URL: https://github.com/apache/kafka/pull/21013 -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2585464467
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
Review Comment:
yes, sure
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
squah-confluent commented on PR #21013: URL: https://github.com/apache/kafka/pull/21013#issuecomment-3607187825 @AntonVasant Thanks for making the changes. Looks good! Could you respond to my comment here when you have time? https://github.com/apache/kafka/pull/21013#discussion_r2581329996 -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
squah-confluent commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2581329996
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
Review Comment:
thanks for making the changes. do you mind also doing the reordering I
suggested earlier?
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2581061484
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
Review Comment:
all the changes have been made as per the comments
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2581058773
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
+String groupId = "fooup";
+String memberId1 = Uuid.randomUuid().toString();
+
+Uuid fooTopicId = Uuid.randomUuid();
+String fooTopicName = "foo";
+Uuid barTopicId = Uuid.randomUuid();
+String barTopicName = "bar";
Review Comment:
yeah, it doesn't make a difference
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013: URL: https://github.com/apache/kafka/pull/21013#discussion_r2580998326 ## group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java: ## @@ -3307,13 +3307,16 @@ private UpdateRegularExpressionsResult maybeUpdateRegularExpressions( // We also trigger a refresh of the regexes in order to resolve it. throwIfRegularExpressionIsInvalid(updatedMember.subscribedTopicRegex()); requireRefresh = true; + Review Comment: sure -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2580997502
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
Review Comment:
yeah thats correct
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
squah-confluent commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2580486043
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
Review Comment:
We aren't really joining a new member in this test, only updating an
existing member. Maybe we could we rename this to
`testConsumerGroupMemberClearsRegex` and move it after
`testConsumerGroupMemberJoinRefreshesExpiredRegexes`?
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
squah-confluent commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2580486043
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
Review Comment:
Could we rename this to `testConsumerGroupMemberClearsRegex` and move it
after `testConsumerGroupMemberJoinRefreshesExpiredRegexes`?
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -21009,6 +21009,90 @@ fooTopicName, computeTopicHash(fooTopicName,
metadataImage)
tasks
);
}
+
+@Test
+public void testConsumerGroupMemberJoinsWithNonEmptyRegexToEmptyRegex() {
+String groupId = "fooup";
+String memberId1 = Uuid.randomUuid().toString();
+
+Uuid fooTopicId = Uuid.randomUuid();
+String fooTopicName = "foo";
+Uuid barTopicId = Uuid.randomUuid();
+String barTopicName = "bar";
Review Comment:
Is `bar` used anywhere? We could remove it if not.
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##
@@ -3307,13 +3307,16 @@ private UpdateRegularExpressionsResult
maybeUpdateRegularExpressions(
// We also trigger a refresh of the regexes in order to
resolve it.
throwIfRegularExpressionIsInvalid(updatedMember.subscribedTopicRegex());
requireRefresh = true;
+
Review Comment:
nit: Could we revert this change?
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
dajac commented on PR #21013: URL: https://github.com/apache/kafka/pull/21013#issuecomment-3591052262 @AntonVasant Could you please add a unit test to cover the change? -- 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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2570553060
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##
@@ -3307,6 +3307,10 @@ private UpdateRegularExpressionsResult
maybeUpdateRegularExpressions(
// We also trigger a refresh of the regexes in order to
resolve it.
throwIfRegularExpressionIsInvalid(updatedMember.subscribedTopicRegex());
requireRefresh = true;
+
+if (isNotEmpty(oldSubscribedTopicRegex) &&
group.numSubscribedMembers(oldSubscribedTopicRegex) != 0) {
+updateRegularExpressionsResult =
UpdateRegularExpressionsResult.REGEX_UPDATED_AND_RESOLVED;
+}
Review Comment:
got it, will make the changes accordingly
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
squah-confluent commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2570546772
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##
@@ -3307,6 +3307,10 @@ private UpdateRegularExpressionsResult
maybeUpdateRegularExpressions(
// We also trigger a refresh of the regexes in order to
resolve it.
throwIfRegularExpressionIsInvalid(updatedMember.subscribedTopicRegex());
requireRefresh = true;
+
+if (isNotEmpty(oldSubscribedTopicRegex) &&
group.numSubscribedMembers(oldSubscribedTopicRegex) != 0) {
+updateRegularExpressionsResult =
UpdateRegularExpressionsResult.REGEX_UPDATED_AND_RESOLVED;
+}
Review Comment:
Yes. When we trigger a refresh we do not want to bump the group epoch until
the refresh is complete. The refresh will bump the group epoch for us once it
has mapped the regex to a list of topics.
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2570542179
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##
@@ -3307,6 +3307,10 @@ private UpdateRegularExpressionsResult
maybeUpdateRegularExpressions(
// We also trigger a refresh of the regexes in order to
resolve it.
throwIfRegularExpressionIsInvalid(updatedMember.subscribedTopicRegex());
requireRefresh = true;
+
+if (isNotEmpty(oldSubscribedTopicRegex) &&
group.numSubscribedMembers(oldSubscribedTopicRegex) != 0) {
+updateRegularExpressionsResult =
UpdateRegularExpressionsResult.REGEX_UPDATED_AND_RESOLVED;
+}
Review Comment:
and not the subscribers counts is empty for the new regex?
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2570540508
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -1464,8 +1464,8 @@ public void
testStaticMemberGetsBackAssignmentUponRejoin() {
.setSubscribedTopicNames(List.of("foo", "bar"))
.setServerAssignorName("range")
.setAssignedPartitions(mkAssignment(
-mkTopicAssignment(fooTopicId, 3, 4, 5),
-mkTopicAssignment(barTopicId, 2)))
+mkTopicAssignment(fooTopicId, 3, 4, 5),
+mkTopicAssignment(barTopicId, 2)))
Review Comment:
sure, correcting this
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
AntonVasant commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2570541352
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##
@@ -3307,6 +3307,10 @@ private UpdateRegularExpressionsResult
maybeUpdateRegularExpressions(
// We also trigger a refresh of the regexes in order to
resolve it.
throwIfRegularExpressionIsInvalid(updatedMember.subscribedTopicRegex());
requireRefresh = true;
+
+if (isNotEmpty(oldSubscribedTopicRegex) &&
group.numSubscribedMembers(oldSubscribedTopicRegex) != 0) {
+updateRegularExpressionsResult =
UpdateRegularExpressionsResult.REGEX_UPDATED_AND_RESOLVED;
+}
Review Comment:
does this mean the regex itself is empty?
--
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]
Re: [PR] KAFKA-19891: Bump group epoch when member regex subscription transitions from non-empty to empty [kafka]
squah-confluent commented on code in PR #21013:
URL: https://github.com/apache/kafka/pull/21013#discussion_r2570523621
##
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##
@@ -3307,6 +3307,10 @@ private UpdateRegularExpressionsResult
maybeUpdateRegularExpressions(
// We also trigger a refresh of the regexes in order to
resolve it.
throwIfRegularExpressionIsInvalid(updatedMember.subscribedTopicRegex());
requireRefresh = true;
+
+if (isNotEmpty(oldSubscribedTopicRegex) &&
group.numSubscribedMembers(oldSubscribedTopicRegex) != 0) {
+updateRegularExpressionsResult =
UpdateRegularExpressionsResult.REGEX_UPDATED_AND_RESOLVED;
+}
Review Comment:
We're changing the return value in the wrong branch of the code. We want the
branch where `newSubscribedTopicRegex` is empty.
##
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java:
##
@@ -1464,8 +1464,8 @@ public void
testStaticMemberGetsBackAssignmentUponRejoin() {
.setSubscribedTopicNames(List.of("foo", "bar"))
.setServerAssignorName("range")
.setAssignedPartitions(mkAssignment(
-mkTopicAssignment(fooTopicId, 3, 4, 5),
-mkTopicAssignment(barTopicId, 2)))
+mkTopicAssignment(fooTopicId, 3, 4, 5),
+mkTopicAssignment(barTopicId, 2)))
Review Comment:
Could we avoid changing the formatting of unrelated tests?
--
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]
