MINOR: Speed up JoinGroupRequestTest, ListGroupsRequestTest, SyncGroupRequestTest by decreased rebalance deplay from 3s to 1s #16810

m1a2st · 2024-08-06T12:57:49Z

Due to this test is too slow, it can't test in 30 seconds. Thus I add a config to speed up this test, let it can completed in 30s

Committer Checklist (excluded from commit message)

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

lianetm · 2024-08-07T19:15:54Z

core/src/test/scala/unit/kafka/server/JoinGroupRequestTest.scala

 @ExtendWith(value = Array(classOf[ClusterTestExtensions]))
 class JoinGroupRequestTest(cluster: ClusterInstance) extends GroupCoordinatorBaseRequestTest(cluster) {
  @ClusterTest(types = Array(Type.KRAFT), serverProperties = Array(
    new ClusterConfigProperty(key = "group.coordinator.new.enable", value = "true"),
    new ClusterConfigProperty(key = "offsets.topic.num.partitions", value = "1"),
-    new ClusterConfigProperty(key = "offsets.topic.replication.factor", value = "1")
+    new ClusterConfigProperty(key = "offsets.topic.replication.factor", value = "1"),
+    new ClusterConfigProperty(key = "group.initial.rebalance.delay.ms", value = "0"),


Interesting finding, makes sense to me for the test. I expect it would it be helpful to add it also in the other test in this file, right?

You are right, but we test these tests without add group.initial.rebalance.delay.ms and find a problem that Kraft mode is obviously slower than Zookeeper mode, thus we guess that root cause is the metadata synchronization speed.

dajac · 2024-08-08T11:16:20Z

@m1a2st Thanks for the patch. I understand the motivation but I wonder if it is a good choice. If we change the configuration, it means that the integration test won’t cover the initial rebalance delay which is the default behavior. I think that we should rather keep it as it is.

m1a2st · 2024-08-08T12:27:25Z

Thanks for your comments, however I watch the kafka document

A new config, group.initial.rebalance.delay.ms was introduced. This config specifies the time, in milliseconds, that the GroupCoordinator will delay the initial consumer rebalance. The rebalance will be further delayed by the value of group.initial.rebalance.delay.ms as new members join the group, up to a maximum of max.poll.interval.ms. The default value for this is 3 seconds. During development and testing it might be desirable to set this to 0 in order to not delay test execution time.

According to the document, thus I consider that these tests are suitable for this config. WDYT?

chia7712 · 2024-08-08T20:05:40Z

The test sends many JoinGroupRequest and each request needs to wait delay ("3" seconds). Also, the zk mode runs so fast due to initial_delay=0 (see following link)

kafka/core/src/test/scala/unit/kafka/utils/TestUtils.scala

Line 352 in e7317b3

    
           if (!props.containsKey(GroupCoordinatorConfig.GROUP_INITIAL_REBALANCE_DELAY_MS_CONFIG))

Maybe we can reduce the delay from 3 seconds to 1.5 seconds? That not only keep the initial rebalance delay but also speed up the test.

chia7712 · 2024-08-10T18:55:56Z

@m1a2st Could you please share the "before" and "after"?

m1a2st · 2024-08-11T03:13:13Z

KafkaServerKRaftRegistrationTest
- Before:
- After:
ConsumerProtocolMigrationTest
- Before:
- After:
JoinGroupRequestTest
- Before:
- After:
ListGroupsRequestTest
- Before:
- After:
OffsetFetchRequestTest
- Before:
- After:
SyncGroupRequestTest
- Before:
- After:

m1a2st · 2024-08-11T03:16:36Z

According to the above test results, I consider we should modify JoinGroupRequestTest, ListGroupsRequestTest and SyncGroupRequestTest by add group.initial.rebalance.delay.ms config, and others should reduce the timeout from 120s to 30s

chia7712 · 2024-08-11T11:34:53Z

According to the above test results, I consider we should modify JoinGroupRequestTest, ListGroupsRequestTest and SyncGroupRequestTest by add group.initial.rebalance.delay.ms config

that makes sense to me

chia7712 · 2024-08-11T11:35:13Z

@m1a2st could you please revise the title? thanks!

chia7712

@m1a2st thanks for updated PR

chia7712 · 2024-08-12T00:18:46Z

core/src/test/scala/integration/kafka/server/KafkaServerKRaftRegistrationTest.scala

@@ -41,7 +41,7 @@ import scala.jdk.CollectionConverters._
 * failure paths is to use timeouts. See {@link unit.kafka.server.BrokerRegistrationRequestTest} for integration test
 * of just the broker registration path.
 */
-@Timeout(120)
+@Timeout(30)


please revert this unrelated change

chia7712 · 2024-08-12T00:18:49Z

core/src/test/scala/unit/kafka/server/ConsumerProtocolMigrationTest.scala

@@ -28,7 +28,7 @@ import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Timeout
 import org.junit.jupiter.api.extension.ExtendWith

-@Timeout(120)
+@Timeout(30)


please revert this unrelated change

chia7712 · 2024-08-12T00:18:58Z

core/src/test/scala/unit/kafka/server/OffsetFetchRequestTest.scala

@@ -29,7 +29,7 @@ import org.junit.jupiter.api.extension.ExtendWith

 import scala.jdk.CollectionConverters._

-@Timeout(120)
+@Timeout(30)


please revert this unrelated change

m1a2st · 2024-08-12T00:23:01Z

@chia7712, Thanks for your comments, revert it.

chia7712

@m1a2st thanks for this patch. please attach the results again as the updated config is incorrect

chia7712 · 2024-08-15T15:51:58Z

core/src/test/scala/unit/kafka/server/JoinGroupRequestTest.scala

@@ -51,7 +52,8 @@ class JoinGroupRequestTest(cluster: ClusterInstance) extends GroupCoordinatorBas
  @ClusterTest(serverProperties = Array(
    new ClusterConfigProperty(key = "group.coordinator.new.enable", value = "false"),
    new ClusterConfigProperty(key = "offsets.topic.num.partitions", value = "1"),
-    new ClusterConfigProperty(key = "offsets.topic.replication.factor", value = "1")
+    new ClusterConfigProperty(key = "offsets.topic.replication.factor", value = "1"),
+    new ClusterConfigProperty(key = "group.initial.rebalance.delay.ms", value = "1"),


it should be "1000" rather than "1"

Yes, the unit is ms, I fix it.

chia7712 · 2024-08-18T14:41:56Z

@m1a2st could you please share the results again?

m1a2st · 2024-08-19T01:10:54Z

The test results:

JoinGroupRequestTest
ListGroupsRequestTest
SyncGroupRequestTest

m1a2st · 2024-08-20T00:47:28Z

JoinGroupRequestTest

test name	type	time(Before)	time(After)
JoinGroupRequestTest		3m 21s	1m 43s
testJoinGroupWithOldConsumerGroupProtocolAndNewGroupCoordinator	[1] Type=Raft-Isolated, MetadataVersion=4.0-IV1, Security=PLAINTEXT	33s 328ms	13s 557ms
testJoinGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator	[1] Type=ZK, MetadataVersion=4.0-IV1, Security=PLAINTEXT	2s 597ms	13s 49ms
testJoinGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator	[2] Type=Raft-Isolated, MetadataVersion=4.0-IV1, Security=PLAINTEXT	32s 596ms	12s 324ms
testJoinGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator	[3] Type=Raft-Combined, MetadataVersion=4.0-IV1, Security=PLAINTEXT	31s 986ms	12s 329ms

ListGroupsRequestTest

test name	type	time(Before)	time(After)
ListGroupsRequestTest		7m 31s	3m 18s
testListGroupsWithNewConsumerGroupProtocolAndNewGroupCoordinator	[1] Type=Raft-Isolated, MetadataVersion=4.0-IVO, Security=PLAINTEXT	57s 215ms	21s 428ms
testListGroupsWithOldConsumerGroupProtocolAndNewGroupCoordinator	[1] Type=Raft-Isolated, MetadataVersion=4.0-IVO, Security=PLAINTEXT	55s 929ms	19s 735ms
testListGroupsWithOldConsumerGroupProtocolAndOldGroupCoordinator	[1] Type=ZK, MetadataVersion=4.0-IVO, Security=PLAINTEXT	1s 731ms	20s 32ms
testListGroupsWithOldConsumerGroupProtocolAndOldGroupCoordinator	[2] Type=Raft-Isolated, MetadataVersion=4.0-IVO, Security=PLAINTEXT	55s 362ms	19s 34ms
testListGroupsWithOldConsumerGroupProtocolAndOldGroupCoordinator	[3] Type=Raft-Combined, MetadataVersion=4.0-IVO, Security=PLAINTEXT	55s 3ms	18s 945ms

SyncGroupRequestTest

test name	type	time(Before)	time(After)
SyncGroupRequestTest		2m 3s	1m 4s
testSyncGroupWithOldConsumerGroupProtocolAndNewGroupCoordinator	[1] Type=Raft-Isolated, MetadataVersion=4.0-IVO, Security=PLAINTEXT	20s 411ms	8s 587ms
testSyncGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator	[1] Type=ZK, MetadataVersion=4.0-IVO, Security=PLAINTEXT	1s 849ms	8s 248ms
testSyncGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator	[2] Type=Raft-Isolated, MetadataVersion=4.0-IVO, Security=PLAINTEXT	19s 753ms	7s 728ms
testSyncGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator	[3] Type=Raft-Combined, MetadataVersion=4.0-IVO, Security=PLAINTEXT	19s 555ms	7s 374ms

chia7712 · 2024-08-20T01:41:15Z

@m1a2st there are some tests getting slower. testJoinGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator, testListGroupsWithOldConsumerGroupProtocolAndOldGroupCoordinator, and testSyncGroupWithOldConsumerGroupProtocolAndOldGroupCoordinator. Do you know the root cause?

m1a2st · 2024-08-20T02:01:09Z

Becasue It is Zookeeper mode test, the config group.initial.rebalance.delay.ms in zookeeper mode default is 0 in

kafka/core/src/test/scala/unit/kafka/utils/TestUtils.scala

Line 354 in 4e1a015

    
           if (!props.containsKey(GroupCoordinatorConfig.GROUP_INITIAL_REBALANCE_DELAY_MS_CONFIG))

thus In these cases that we transfer to 1000 is slower than before. In KRaft mode it doesn't init the config value, thus the defulat value is 3000.

chia7712 · 2024-08-20T02:08:06Z

thus In these cases that we transfer to 1000 is slower than before. In KRaft mode it doesn't init the config value, thus the defulat value is 3000.

makes sense. LGTM. will merge it later

chia7712

LGTM

chia7712 · 2024-08-20T02:10:12Z

@m1a2st could you please rebase code to trigger QA again? there are many failed tests. I feel they are unrelated, but double-check is not bad thing :)

m1a2st · 2024-08-20T02:12:59Z

I have rebased it :)

chia7712 · 2024-08-22T05:19:26Z

@m1a2st please fix conflicts

m1a2st · 2024-08-22T05:41:59Z

Thanks for your comments. I resolve it.

chia7712 · 2024-08-22T08:04:56Z

With merging this PR we can keep testing the global timeout for all IT (#16957)

…pRequestTest by decreased rebalance deplay from 3s to 1s (apache#16810) Reviewers: Chia-Ping Tsai <[email protected]>

lianetm reviewed Aug 7, 2024

View reviewed changes

lianetm added the tests Test fixes (including flaky tests) label Aug 7, 2024

m1a2st force-pushed the MINOR-JoinGroupRequestTEst branch 2 times, most recently from 1230c8f to 39e2dbb Compare August 10, 2024 06:32

m1a2st force-pushed the MINOR-JoinGroupRequestTEst branch from 39e2dbb to b25234f Compare August 11, 2024 03:20

m1a2st changed the title ~~MINOR: Speed up JoinGroupRequestTest#testJoinGroupWithOldConsumerGroupProtocolAndNewGroupCoordinator~~ MINOR: Speed up JoinGroupRequestTest, ListGroupsRequestTest, SyncGroupRequestTest by decreased rebalance deplay from 3s to 1s Aug 11, 2024

m1a2st force-pushed the MINOR-JoinGroupRequestTEst branch from b25234f to 9aedd6a Compare August 11, 2024 11:56

chia7712 reviewed Aug 12, 2024

View reviewed changes

m1a2st force-pushed the MINOR-JoinGroupRequestTEst branch from 9aedd6a to f36ac96 Compare August 12, 2024 00:22

chia7712 reviewed Aug 15, 2024

View reviewed changes

chia7712 approved these changes Aug 20, 2024

View reviewed changes

m1a2st force-pushed the MINOR-JoinGroupRequestTEst branch from b77077a to d3bb650 Compare August 20, 2024 02:11

add group.initial.rebalance.delay.ms config in JoinGroupRequestTest

3f2d0e7

m1a2st force-pushed the MINOR-JoinGroupRequestTEst branch from d3bb650 to 3f2d0e7 Compare August 22, 2024 05:28

rebased it

e87bb98

fix ListGroupsRequestTest

d11821c

chia7712 merged commit 61a661e into apache:trunk Aug 22, 2024
5 of 6 checks passed

m1a2st mentioned this pull request Aug 22, 2024

KAFKA-16813: Add global timeout for @ClusterTemplate, @ClusterTest and @ClusterTests #16957

Merged

3 tasks

m1a2st mentioned this pull request Sep 11, 2024

KAFKA-17472: Speed Up DescribeConsumerGroupTest #17117

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Speed up JoinGroupRequestTest, ListGroupsRequestTest, SyncGroupRequestTest by decreased rebalance deplay from 3s to 1s #16810

MINOR: Speed up JoinGroupRequestTest, ListGroupsRequestTest, SyncGroupRequestTest by decreased rebalance deplay from 3s to 1s #16810

m1a2st commented Aug 6, 2024

lianetm Aug 7, 2024

m1a2st Aug 8, 2024 •

edited

Loading

dajac commented Aug 8, 2024

m1a2st commented Aug 8, 2024 •

edited

Loading

chia7712 commented Aug 8, 2024

chia7712 commented Aug 10, 2024

m1a2st commented Aug 11, 2024

m1a2st commented Aug 11, 2024

chia7712 commented Aug 11, 2024

chia7712 commented Aug 11, 2024

chia7712 left a comment

chia7712 Aug 12, 2024

chia7712 Aug 12, 2024

chia7712 Aug 12, 2024

m1a2st commented Aug 12, 2024

chia7712 left a comment

chia7712 Aug 15, 2024

m1a2st Aug 15, 2024

chia7712 commented Aug 18, 2024

m1a2st commented Aug 19, 2024

m1a2st commented Aug 20, 2024 •

edited

Loading

chia7712 commented Aug 20, 2024

m1a2st commented Aug 20, 2024 •

edited

Loading

chia7712 commented Aug 20, 2024

chia7712 left a comment

chia7712 commented Aug 20, 2024

m1a2st commented Aug 20, 2024

chia7712 commented Aug 22, 2024

m1a2st commented Aug 22, 2024

chia7712 commented Aug 22, 2024

MINOR: Speed up JoinGroupRequestTest, ListGroupsRequestTest, SyncGroupRequestTest by decreased rebalance deplay from 3s to 1s #16810

MINOR: Speed up JoinGroupRequestTest, ListGroupsRequestTest, SyncGroupRequestTest by decreased rebalance deplay from 3s to 1s #16810

Conversation

m1a2st commented Aug 6, 2024

Committer Checklist (excluded from commit message)

lianetm Aug 7, 2024

Choose a reason for hiding this comment

m1a2st Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

dajac commented Aug 8, 2024

m1a2st commented Aug 8, 2024 • edited Loading

chia7712 commented Aug 8, 2024

chia7712 commented Aug 10, 2024

m1a2st commented Aug 11, 2024

m1a2st commented Aug 11, 2024

chia7712 commented Aug 11, 2024

chia7712 commented Aug 11, 2024

chia7712 left a comment

Choose a reason for hiding this comment

chia7712 Aug 12, 2024

Choose a reason for hiding this comment

chia7712 Aug 12, 2024

Choose a reason for hiding this comment

chia7712 Aug 12, 2024

Choose a reason for hiding this comment

m1a2st commented Aug 12, 2024

chia7712 left a comment

Choose a reason for hiding this comment

chia7712 Aug 15, 2024

Choose a reason for hiding this comment

m1a2st Aug 15, 2024

Choose a reason for hiding this comment

chia7712 commented Aug 18, 2024

m1a2st commented Aug 19, 2024

m1a2st commented Aug 20, 2024 • edited Loading

chia7712 commented Aug 20, 2024

m1a2st commented Aug 20, 2024 • edited Loading

chia7712 commented Aug 20, 2024

chia7712 left a comment

Choose a reason for hiding this comment

chia7712 commented Aug 20, 2024

m1a2st commented Aug 20, 2024

chia7712 commented Aug 22, 2024

m1a2st commented Aug 22, 2024

chia7712 commented Aug 22, 2024

m1a2st Aug 8, 2024 •

edited

Loading

m1a2st commented Aug 8, 2024 •

edited

Loading

m1a2st commented Aug 20, 2024 •

edited

Loading

m1a2st commented Aug 20, 2024 •

edited

Loading