MINOR: migrate DescribeConsumerGroupTest to use ClusterTestExtensions #15908

FrankYang0529 · 2024-05-09T15:24:17Z

By using ClusterTestExtensions, DescribeConsumerGroupTest get get away from KafkaServerTestHarness dependency.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

showuon · 2024-05-22T05:50:42Z

tools/src/test/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommandTestUtils.java

@@ -92,6 +94,7 @@ static void generator(ClusterGenerator clusterGenerator) {
    static <T> AutoCloseable buildConsumers(int numberOfConsumers,
                                            boolean syncCommit,
                                            String topic,
+                                            Collection<TopicPartition> topicPartitions,


nit: Change to set to be more specific.

showuon · 2024-05-22T05:50:51Z

tools/src/test/java/org/apache/kafka/tools/consumer/group/DeleteConsumerGroupsTest.java

@@ -311,6 +311,7 @@ private AutoCloseable consumerGroupClosable(ClusterInstance cluster, GroupProtoc
                1,
                false,
                topicName,
+                null,


nit: Could we use empty set instead of null here?

showuon · 2024-05-22T06:09:11Z

tools/src/test/java/org/apache/kafka/tools/consumer/group/DescribeConsumerGroupTest.java

+        GroupProtocol groupProtocol = clusterInstance.config().serverProperties().get(GroupCoordinatorConfig.NEW_GROUP_COORDINATOR_ENABLE_CONFIG).trim().equals("true") ? GroupProtocol.CONSUMER : GroupProtocol.CLASSIC;
+        try (AutoCloseable protocolConsumerGroupExecutor = consumerGroupClosable(groupProtocol, PROTOCOL_GROUP, TOPIC, Collections.emptyMap());


It looks to me that we could put the logic of determining groupProtocol in consumerGroupClosable method, so that we don't have to do the same thing in each test. Ex: Add a consumerGroupClosable method, and replace the groupProtocol with clusterInstance.config().serverProperties(). So we can do the groupProtocol determination inside the consumerGroupClosable method:

private AutoCloseable consumerGroupClosable(Map<String, String> serverProperties, GroupProtocol protocol, String groupId, String topicName) { GroupProtocol groupProtocol = serverProperties.get(GroupCoordinatorConfig.NEW_GROUP_COORDINATOR_ENABLE_CONFIG).trim().equals("true") ? GroupProtocol.CONSUMER : GroupProtocol.CLASSIC; return consumerGroupClosable(groupProtocol, ...);

Thanks for the suggestion. I found that I only run consumer group protocol when group.coordinator.new.enable is true. I change to use ClusterInstance#supportedGroupProtocols to check supported protocols, so we can run all cases like before. Thanks.

showuon · 2024-05-22T06:19:39Z

One high-level question:
I found after this change, we increased the test case numbers from 492 -> 660. And the time for this test suite takes form 1h 19m -> 1h 31m. Do we really need all these tests? Could we try to:

at least maintain the same test case numbers as before?
(could be in another PR) Try to speed up the test. Maybe disable log.cleaner.enable, deploy for 1 nodes if we are not testing the data replication, or maybe reuse the same cluster for some similar tests... etc?

Anyway, to me, one test class takes more than 1 hour is really unacceptable.

Trunk:
https://ci-builds.apache.org/job/Kafka/job/kafka/job/trunk/2924/testReport/org.apache.kafka.tools.consumer.group/
This PR:
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15908/1/testReport/org.apache.kafka.tools.consumer.group/

showuon · 2024-05-22T06:20:30Z

Forgot to say, thanks for closing all the ConsumerGroupService instances. I can't believe we leak these resources before.

FrankYang0529 · 2024-05-25T13:49:55Z

One high-level question:
I found after this change, we increased the test case numbers from 492 -> 660. And the time for this test suite takes form 1h 19m -> 1h 31m. Do we really need all these tests?

Hi @showuon, thanks for the review and suggestion. Originally, old framework run 4 cases:

(ZK / KRAFT servers) with (group.coordinator.new.enable=false) with (classic group protocol) = 2 cases
(KRAFT server) with (group.coordinator.new.enable=true) with (classic group protocol) = 1 case
(KRAFT server) with (group.coordinator.new.enable=true) with (consumer group protocol) = 1 case

In the new framework, we run 7 cases:

(ZK / KRAFT / CO_KRAFT servers) with (group.coordinator.new.enable=false) with (classic group protocol) = 3 cases
(KRAFT / CO_KRAFT servers) with (group.coordinator.new.enable=true) with (classic group protocol) = 2 cases
(KRAFT / CO_KRAFT servers) with (group.coordinator.new.enable=true) with (consumer group protocol) = 2 cases

That is why testing time takes longer. In my latest update, I change to run following cases. Also, in old framework, we started KRAFT server with group.coordinator.new.enable=true twice. One for classic group protocol and another for consumer group protocol. In new framework, we can get supported group protocol by ClusterInstance#supportedGroupProtocols, so I use one KRAFT server with group.coordinator.new.enable=true to run two protocols. We can reduce time to setup one KRAFT cluster. I hope this change can reduce overall testing time. Thanks.

(ZK / KRAFT servers) with (group.coordinator.new.enable=false) with (classic group protocol) = 2 cases
(KRAFT servers) with (group.coordinator.new.enable=true) with (classic / consumer group protocols) = 2 cases

TaiJuWu · 2024-05-28T13:20:13Z

tools/src/test/java/org/apache/kafka/tools/consumer/group/DescribeConsumerGroupTest.java

-
+import java.util.stream.Stream;
+
+//import static kafka.test.annotation.Type.CO_KRAFT;


should this mark be removed?

Removed it. Thanks.

chia7712 · 2024-05-29T17:43:19Z

That is why testing time takes longer. In my latest update, I change to run following cases. Also, in old framework, we started KRAFT server with group.coordinator.new.enable=true twice. One for classic group protocol and another for consumer group protocol. In new framework, we can get supported group protocol by ClusterInstance#supportedGroupProtocols, so I use one KRAFT server with group.coordinator.new.enable=true to run two protocols. We can reduce time to setup one KRAFT cluster. I hope this change can reduce overall testing time. Thanks.
(ZK / KRAFT servers) with (group.coordinator.new.enable=false) with (classic group protocol) = 2 cases
(KRAFT servers) with (group.coordinator.new.enable=true) with (classic / consumer group protocols) = 2 cases

agree. could this PR include this change?

FrankYang0529 · 2024-05-30T01:34:03Z

That is why testing time takes longer. In my latest update, I change to run following cases. Also, in old framework, we started KRAFT server with group.coordinator.new.enable=true twice. One for classic group protocol and another for consumer group protocol. In new framework, we can get supported group protocol by ClusterInstance#supportedGroupProtocols, so I use one KRAFT server with group.coordinator.new.enable=true to run two protocols. We can reduce time to setup one KRAFT cluster. I hope this change can reduce overall testing time. Thanks.
(ZK / KRAFT servers) with (group.coordinator.new.enable=false) with (classic group protocol) = 2 cases
(KRAFT servers) with (group.coordinator.new.enable=true) with (classic / consumer group protocols) = 2 cases

agree. could this PR include this change?

Yes, I already changed it. Please take a look for DescribeConsumerGroupTest#generator. Thanks

Signed-off-by: PoAn Yang <payang@apache.org>

chia7712 · 2024-05-30T13:31:04Z

@FrankYang0529 please fix the conflicts, thanks!

FrankYang0529 mentioned this pull request May 11, 2024

MINOR: migrate ListConsumerGroupTest to use ClusterTestExtensions #15821

Merged

3 tasks

showuon reviewed May 22, 2024

View reviewed changes

FrankYang0529 force-pushed the minor-migrate-DescribeConsumerGroup branch 2 times, most recently from 79dfa65 to 0a6e0ce Compare May 25, 2024 11:10

FrankYang0529 force-pushed the minor-migrate-DescribeConsumerGroup branch 2 times, most recently from fc7ebc5 to 3cbd36d Compare May 28, 2024 08:52

TaiJuWu reviewed May 28, 2024

View reviewed changes

FrankYang0529 force-pushed the minor-migrate-DescribeConsumerGroup branch from 3cbd36d to bcffd08 Compare May 28, 2024 13:47

MINOR: migrate DescribeConsumerGroupTest to use ClusterTestExtensions

1faf820

Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 force-pushed the minor-migrate-DescribeConsumerGroup branch from bcffd08 to 1faf820 Compare May 30, 2024 01:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: migrate DescribeConsumerGroupTest to use ClusterTestExtensions #15908

MINOR: migrate DescribeConsumerGroupTest to use ClusterTestExtensions #15908

FrankYang0529 commented May 9, 2024

showuon May 22, 2024

showuon May 22, 2024

showuon May 22, 2024

FrankYang0529 May 25, 2024

showuon commented May 22, 2024 •

edited

showuon commented May 22, 2024

FrankYang0529 commented May 25, 2024

TaiJuWu May 28, 2024

FrankYang0529 May 28, 2024

chia7712 commented May 29, 2024

FrankYang0529 commented May 30, 2024

chia7712 commented May 30, 2024

		GroupProtocol groupProtocol = clusterInstance.config().serverProperties().get(GroupCoordinatorConfig.NEW_GROUP_COORDINATOR_ENABLE_CONFIG).trim().equals("true") ? GroupProtocol.CONSUMER : GroupProtocol.CLASSIC;
		try (AutoCloseable protocolConsumerGroupExecutor = consumerGroupClosable(groupProtocol, PROTOCOL_GROUP, TOPIC, Collections.emptyMap());


		import java.util.stream.Stream;

		//import static kafka.test.annotation.Type.CO_KRAFT;

MINOR: migrate DescribeConsumerGroupTest to use ClusterTestExtensions #15908

Are you sure you want to change the base?

MINOR: migrate DescribeConsumerGroupTest to use ClusterTestExtensions #15908

Conversation

FrankYang0529 commented May 9, 2024

Committer Checklist (excluded from commit message)

showuon May 22, 2024

Choose a reason for hiding this comment

showuon May 22, 2024

Choose a reason for hiding this comment

showuon May 22, 2024

Choose a reason for hiding this comment

FrankYang0529 May 25, 2024

Choose a reason for hiding this comment

showuon commented May 22, 2024 • edited

showuon commented May 22, 2024

FrankYang0529 commented May 25, 2024

TaiJuWu May 28, 2024

Choose a reason for hiding this comment

FrankYang0529 May 28, 2024

Choose a reason for hiding this comment

chia7712 commented May 29, 2024

FrankYang0529 commented May 30, 2024

chia7712 commented May 30, 2024

showuon commented May 22, 2024 •

edited