dajac commented on code in PR #17958:
URL: https://github.com/apache/kafka/pull/17958#discussion_r1865805655
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestTest.scala:
##########
@@ -213,4 +216,108 @@ class ConsumerGroupDescribeRequestTest(cluster:
ClusterInstance) extends GroupCo
admin.close()
}
}
+
+ @ClusterTest(
+ types = Array(Type.KRAFT),
+ serverProperties = Array(
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_PARTITIONS_CONFIG, value = "1"),
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "1")
+ )
+ )
+ def testConsumerGroupDescribeWithMigrationMember(): Unit = {
+ // Creates the __consumer_offsets topics because it won't be created
automatically
+ // in this test because it does not use FindCoordinator API.
+ createOffsetsTopic()
+
+ // Create the topic.
+ val topicName = "foo"
+ createTopic(
+ topic = topicName,
+ numPartitions = 3
+ )
+
+ val groupId = "grp"
+
+ // Classic member 1 joins the classic group.
+ val memberId1 = joinDynamicConsumerGroupWithOldProtocol(
+ groupId = groupId,
+ metadata = ConsumerProtocol.serializeSubscription(
+ new ConsumerPartitionAssignor.Subscription(
+ Collections.singletonList(topicName),
+ null,
+ List().asJava
+ )
+ ).array,
+ assignment = ConsumerProtocol.serializeAssignment(
+ new ConsumerPartitionAssignor.Assignment(
+ List(0, 1, 2).map(p => new TopicPartition(topicName, p)).asJava
+ )
+ ).array
+ )._1
+
+ // The joining request with a consumer group member 2 is accepted.
+ val memberId2 = consumerGroupHeartbeat(
+ groupId = groupId,
+ memberId = "member-2",
+ rebalanceTimeoutMs = 5 * 60 * 1000,
+ subscribedTopicNames = List(topicName),
+ topicPartitions = List.empty,
+ expectedError = Errors.NONE
+ ).memberId()
Review Comment:
nit: You can omit the `()`.
##########
clients/src/main/resources/common/message/ConsumerGroupDescribeResponse.json:
##########
@@ -69,7 +70,9 @@
{ "name": "Assignment", "type": "Assignment", "versions": "0+",
"about": "The current assignment." },
{ "name": "TargetAssignment", "type": "Assignment", "versions":
"0+",
- "about": "The target assignment." }
+ "about": "The target assignment." },
+ { "name": "IsClassic", "type": "bool", "versions": "1+",
"ignorable": true,
+ "about": "True for classic member." }
Review Comment:
I think that there is one case which is not clear in the KIP. What value
will the admin client return when the version 1 is not available? If the new
version is not available, we don't know whether the member is classic or not.
Hence, we cannot use true or false by default. What's your take on this?
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestTest.scala:
##########
@@ -213,4 +216,108 @@ class ConsumerGroupDescribeRequestTest(cluster:
ClusterInstance) extends GroupCo
admin.close()
}
}
+
+ @ClusterTest(
+ types = Array(Type.KRAFT),
+ serverProperties = Array(
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_PARTITIONS_CONFIG, value = "1"),
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "1")
+ )
+ )
+ def testConsumerGroupDescribeWithMigrationMember(): Unit = {
+ // Creates the __consumer_offsets topics because it won't be created
automatically
+ // in this test because it does not use FindCoordinator API.
+ createOffsetsTopic()
+
+ // Create the topic.
+ val topicName = "foo"
+ createTopic(
+ topic = topicName,
+ numPartitions = 3
+ )
+
+ val groupId = "grp"
+
+ // Classic member 1 joins the classic group.
+ val memberId1 = joinDynamicConsumerGroupWithOldProtocol(
+ groupId = groupId,
+ metadata = ConsumerProtocol.serializeSubscription(
+ new ConsumerPartitionAssignor.Subscription(
+ Collections.singletonList(topicName),
+ null,
+ List().asJava
+ )
+ ).array,
+ assignment = ConsumerProtocol.serializeAssignment(
+ new ConsumerPartitionAssignor.Assignment(
+ List(0, 1, 2).map(p => new TopicPartition(topicName, p)).asJava
+ )
+ ).array
+ )._1
+
+ // The joining request with a consumer group member 2 is accepted.
+ val memberId2 = consumerGroupHeartbeat(
+ groupId = groupId,
+ memberId = "member-2",
+ rebalanceTimeoutMs = 5 * 60 * 1000,
+ subscribedTopicNames = List(topicName),
+ topicPartitions = List.empty,
+ expectedError = Errors.NONE
+ ).memberId()
+
+ for (version <- ApiKeys.CONSUMER_GROUP_DESCRIBE.oldestVersion() to
ApiKeys.CONSUMER_GROUP_DESCRIBE.latestVersion(isUnstableApiEnabled)) {
+ val actual = consumerGroupDescribe(
+ groupIds = List(groupId),
+ includeAuthorizedOperations = true,
+ version = version.toShort,
+ )
+ assertEquals(1, actual.size)
+ val group = actual.head
+ val member1 = group.members().asScala.find(_.memberId() == memberId1)
Review Comment:
nit: You can omit the `()` after `members`.
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestTest.scala:
##########
@@ -213,4 +216,108 @@ class ConsumerGroupDescribeRequestTest(cluster:
ClusterInstance) extends GroupCo
admin.close()
}
}
+
+ @ClusterTest(
+ types = Array(Type.KRAFT),
+ serverProperties = Array(
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_PARTITIONS_CONFIG, value = "1"),
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "1")
+ )
+ )
+ def testConsumerGroupDescribeWithMigrationMember(): Unit = {
+ // Creates the __consumer_offsets topics because it won't be created
automatically
+ // in this test because it does not use FindCoordinator API.
+ createOffsetsTopic()
+
+ // Create the topic.
+ val topicName = "foo"
+ createTopic(
+ topic = topicName,
+ numPartitions = 3
+ )
+
+ val groupId = "grp"
+
+ // Classic member 1 joins the classic group.
+ val memberId1 = joinDynamicConsumerGroupWithOldProtocol(
+ groupId = groupId,
+ metadata = ConsumerProtocol.serializeSubscription(
+ new ConsumerPartitionAssignor.Subscription(
+ Collections.singletonList(topicName),
+ null,
+ List().asJava
+ )
+ ).array,
+ assignment = ConsumerProtocol.serializeAssignment(
+ new ConsumerPartitionAssignor.Assignment(
+ List(0, 1, 2).map(p => new TopicPartition(topicName, p)).asJava
+ )
+ ).array
+ )._1
+
+ // The joining request with a consumer group member 2 is accepted.
+ val memberId2 = consumerGroupHeartbeat(
+ groupId = groupId,
+ memberId = "member-2",
+ rebalanceTimeoutMs = 5 * 60 * 1000,
+ subscribedTopicNames = List(topicName),
+ topicPartitions = List.empty,
+ expectedError = Errors.NONE
+ ).memberId()
+
+ for (version <- ApiKeys.CONSUMER_GROUP_DESCRIBE.oldestVersion() to
ApiKeys.CONSUMER_GROUP_DESCRIBE.latestVersion(isUnstableApiEnabled)) {
+ val actual = consumerGroupDescribe(
+ groupIds = List(groupId),
+ includeAuthorizedOperations = true,
+ version = version.toShort,
+ )
+ assertEquals(1, actual.size)
+ val group = actual.head
+ val member1 = group.members().asScala.find(_.memberId() == memberId1)
+ assertFalse(member1.isEmpty)
+ // Version 0 doesn't have isClassic field, so isClassic field on member
1 is false.
+ // After version 1, there is isClassic field and it should be true for
member 1.
+ assertEquals(version > 0.toShort, member1.get.isClassic)
+
+ val member2 = group.members().asScala.find(_.memberId() == memberId2)
+ assertFalse(member2.isEmpty)
+ assertFalse(member2.get.isClassic)
+ }
+
+ // Classic member 1 leaves group.
+ leaveGroup(
+ groupId = groupId,
+ memberId = memberId1,
+ useNewProtocol = false,
+ version = ApiKeys.LEAVE_GROUP.latestVersion(isUnstableApiEnabled)
+ )
+
+ // Member 1 joins as consumer group member.
+ consumerGroupHeartbeat(
+ groupId = groupId,
+ memberId = memberId1,
+ rebalanceTimeoutMs = 5 * 60 * 1000,
+ subscribedTopicNames = List(topicName),
+ topicPartitions = List.empty,
+ expectedError = Errors.NONE
+ )
+
+ // There is no classic member in the group.
+ for (version <- ApiKeys.CONSUMER_GROUP_DESCRIBE.oldestVersion() to
ApiKeys.CONSUMER_GROUP_DESCRIBE.latestVersion(isUnstableApiEnabled)) {
+ val actual = consumerGroupDescribe(
+ groupIds = List(groupId),
+ includeAuthorizedOperations = true,
+ version = version.toShort,
+ )
+ assertEquals(1, actual.size)
+ val group = actual.head
+ val member1 = group.members().asScala.find(_.memberId() == memberId1)
Review Comment:
ditto.
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestTest.scala:
##########
@@ -213,4 +216,108 @@ class ConsumerGroupDescribeRequestTest(cluster:
ClusterInstance) extends GroupCo
admin.close()
}
}
+
+ @ClusterTest(
+ types = Array(Type.KRAFT),
+ serverProperties = Array(
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_PARTITIONS_CONFIG, value = "1"),
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "1")
+ )
+ )
+ def testConsumerGroupDescribeWithMigrationMember(): Unit = {
+ // Creates the __consumer_offsets topics because it won't be created
automatically
+ // in this test because it does not use FindCoordinator API.
+ createOffsetsTopic()
+
+ // Create the topic.
+ val topicName = "foo"
+ createTopic(
+ topic = topicName,
+ numPartitions = 3
+ )
+
+ val groupId = "grp"
+
+ // Classic member 1 joins the classic group.
+ val memberId1 = joinDynamicConsumerGroupWithOldProtocol(
+ groupId = groupId,
+ metadata = ConsumerProtocol.serializeSubscription(
+ new ConsumerPartitionAssignor.Subscription(
+ Collections.singletonList(topicName),
+ null,
+ List().asJava
+ )
+ ).array,
+ assignment = ConsumerProtocol.serializeAssignment(
+ new ConsumerPartitionAssignor.Assignment(
+ List(0, 1, 2).map(p => new TopicPartition(topicName, p)).asJava
+ )
+ ).array
+ )._1
+
+ // The joining request with a consumer group member 2 is accepted.
+ val memberId2 = consumerGroupHeartbeat(
+ groupId = groupId,
+ memberId = "member-2",
+ rebalanceTimeoutMs = 5 * 60 * 1000,
+ subscribedTopicNames = List(topicName),
+ topicPartitions = List.empty,
+ expectedError = Errors.NONE
+ ).memberId()
+
+ for (version <- ApiKeys.CONSUMER_GROUP_DESCRIBE.oldestVersion() to
ApiKeys.CONSUMER_GROUP_DESCRIBE.latestVersion(isUnstableApiEnabled)) {
+ val actual = consumerGroupDescribe(
+ groupIds = List(groupId),
+ includeAuthorizedOperations = true,
+ version = version.toShort,
+ )
+ assertEquals(1, actual.size)
+ val group = actual.head
+ val member1 = group.members().asScala.find(_.memberId() == memberId1)
+ assertFalse(member1.isEmpty)
+ // Version 0 doesn't have isClassic field, so isClassic field on member
1 is false.
+ // After version 1, there is isClassic field and it should be true for
member 1.
+ assertEquals(version > 0.toShort, member1.get.isClassic)
+
+ val member2 = group.members().asScala.find(_.memberId() == memberId2)
+ assertFalse(member2.isEmpty)
+ assertFalse(member2.get.isClassic)
+ }
+
+ // Classic member 1 leaves group.
+ leaveGroup(
+ groupId = groupId,
+ memberId = memberId1,
+ useNewProtocol = false,
+ version = ApiKeys.LEAVE_GROUP.latestVersion(isUnstableApiEnabled)
+ )
+
+ // Member 1 joins as consumer group member.
+ consumerGroupHeartbeat(
+ groupId = groupId,
+ memberId = memberId1,
+ rebalanceTimeoutMs = 5 * 60 * 1000,
+ subscribedTopicNames = List(topicName),
+ topicPartitions = List.empty,
+ expectedError = Errors.NONE
+ )
+
+ // There is no classic member in the group.
+ for (version <- ApiKeys.CONSUMER_GROUP_DESCRIBE.oldestVersion() to
ApiKeys.CONSUMER_GROUP_DESCRIBE.latestVersion(isUnstableApiEnabled)) {
+ val actual = consumerGroupDescribe(
+ groupIds = List(groupId),
+ includeAuthorizedOperations = true,
+ version = version.toShort,
+ )
+ assertEquals(1, actual.size)
+ val group = actual.head
+ val member1 = group.members().asScala.find(_.memberId() == memberId1)
+ assertFalse(member1.isEmpty)
+ assertFalse(member1.get.isClassic)
+
+ val member2 = group.members().asScala.find(_.memberId() == memberId2)
Review Comment:
ditto.
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestTest.scala:
##########
@@ -213,4 +216,108 @@ class ConsumerGroupDescribeRequestTest(cluster:
ClusterInstance) extends GroupCo
admin.close()
}
}
+
+ @ClusterTest(
+ types = Array(Type.KRAFT),
+ serverProperties = Array(
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_PARTITIONS_CONFIG, value = "1"),
+ new ClusterConfigProperty(key =
GroupCoordinatorConfig.OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "1")
+ )
+ )
+ def testConsumerGroupDescribeWithMigrationMember(): Unit = {
+ // Creates the __consumer_offsets topics because it won't be created
automatically
+ // in this test because it does not use FindCoordinator API.
+ createOffsetsTopic()
+
+ // Create the topic.
+ val topicName = "foo"
+ createTopic(
+ topic = topicName,
+ numPartitions = 3
+ )
+
+ val groupId = "grp"
+
+ // Classic member 1 joins the classic group.
+ val memberId1 = joinDynamicConsumerGroupWithOldProtocol(
+ groupId = groupId,
+ metadata = ConsumerProtocol.serializeSubscription(
+ new ConsumerPartitionAssignor.Subscription(
+ Collections.singletonList(topicName),
+ null,
+ List().asJava
+ )
+ ).array,
+ assignment = ConsumerProtocol.serializeAssignment(
+ new ConsumerPartitionAssignor.Assignment(
+ List(0, 1, 2).map(p => new TopicPartition(topicName, p)).asJava
+ )
+ ).array
+ )._1
+
+ // The joining request with a consumer group member 2 is accepted.
+ val memberId2 = consumerGroupHeartbeat(
+ groupId = groupId,
+ memberId = "member-2",
+ rebalanceTimeoutMs = 5 * 60 * 1000,
+ subscribedTopicNames = List(topicName),
+ topicPartitions = List.empty,
+ expectedError = Errors.NONE
+ ).memberId()
+
+ for (version <- ApiKeys.CONSUMER_GROUP_DESCRIBE.oldestVersion() to
ApiKeys.CONSUMER_GROUP_DESCRIBE.latestVersion(isUnstableApiEnabled)) {
+ val actual = consumerGroupDescribe(
+ groupIds = List(groupId),
+ includeAuthorizedOperations = true,
+ version = version.toShort,
+ )
+ assertEquals(1, actual.size)
+ val group = actual.head
+ val member1 = group.members().asScala.find(_.memberId() == memberId1)
+ assertFalse(member1.isEmpty)
+ // Version 0 doesn't have isClassic field, so isClassic field on member
1 is false.
+ // After version 1, there is isClassic field and it should be true for
member 1.
+ assertEquals(version > 0.toShort, member1.get.isClassic)
+
+ val member2 = group.members().asScala.find(_.memberId() == memberId2)
Review Comment:
ditto.
--
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
