MINOR: Split up ShareConsumerTest by AndrewJSchofield · Pull Request #22017 · apache/kafka

AndrewJSchofield · 2026-04-09T22:21:17Z

Split up the excessively large ShareConsumerTest into a handful of
smaller source files.

Reviewers: Chia-Ping Tsai chia7712@gmail.com

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java

see-quick · 2026-04-10T13:16:42Z

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java

+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@Timeout(1200)
+@Tag("integration")


Do we need to add @Tag("integration") here? as I can see in @ClusterTest already covers this and is applied consistently across other test suites.

This one was an outlier and I'm not really sure why. I've added it.

yes, it is redundant

chia7712

@AndrewJSchofield thanks for this excellent refactor

chia7712 · 2026-04-10T15:08:58Z

...nts-integration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerTest.java

@@ -131,7 +97,6 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;

-@SuppressWarnings({"ClassFanOutComplexity", "ClassDataAbstractionCoupling"})
 @Timeout(1200)
 @Tag("integration")


ClusterTest covers this tag out of box, so we are good to skip it here

chia7712 · 2026-04-10T15:09:33Z

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java

+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@Timeout(1200)
+@Tag("integration")


yes, it is redundant

chia7712 · 2026-04-10T15:09:38Z

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerDeliveryTest.java

+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@Timeout(1200)
+@Tag("integration")


chia7712 · 2026-04-10T15:09:44Z

...-integration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerLagTest.java

+import static org.junit.jupiter.api.Assertions.fail;
+
+@Timeout(1200)
+@Tag("integration")


chia7712 · 2026-04-10T15:09:50Z

...ration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerRackAwareTest.java

-        }
-    )
-    public static class WithAssignmentBatchingDisabledTest extends ShareConsumerRackAwareTest {
+@Tag("integration")


chia7712 · 2026-04-10T15:09:54Z

...ntegration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerRenewTest.java

+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@Timeout(1200)
+@Tag("integration")


chia7712 · 2026-04-10T15:10:32Z

...integration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerTestBase.java

+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class ShareConsumerTestBase {


Should it be abstract?

chia7712 · 2026-04-10T15:12:35Z

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java

+            assertEquals(1, records.count());
+
+            // Waiting until the acquisition lock expires.
+            Thread.sleep(20000);


Can we use a smaller group.share.record.lock.duration.ms here to make the test run faster?

It's taken effort to get these tests to run reliably so I'm nervous about messing with the timeouts. It's a bit unpredictable ensuring that the timeout only happens when we want it to. Luckily there are few tests that wait like this.

see-quick

👍, btw , I think there may be additional places in the integration tests where we are doing this redundantly (i.e., tagging integration) . It might be worth opening an issue so someone can clean it up and we can avoid these niche definitions. WDYT?

chia7712 · 2026-04-12T11:10:36Z

It might be worth opening an issue so someone can clean it up and we can avoid these niche definitions. WDYT?

That's a great idea! If you have some free cycles, please feel free to open a minor PR to address it. :)

see-quick · 2026-04-13T07:03:18Z

It might be worth opening an issue so someone can clean it up and we can avoid these niche definitions. WDYT?

That's a great idea! If you have some free cycles, please feel free to open a minor PR to address it. :)

I will try this week if I found some spare time :)) Thanks.

chia7712

@AndrewJSchofield thanks for this patch. a couple of comments left

chia7712 · 2026-04-14T06:48:51Z

...ration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerRackAwareTest.java

-
-    @ClusterTestDefaults(
-        serverProperties = {
-            @ClusterConfigProperty(key = GroupCoordinatorConfig.SHARE_GROUP_ASSIGNMENT_INTERVAL_MS_CONFIG, value = "0")


It looks like this test case was removed. Was this intentional, or an accidental deletion

It had an empty body. I do not understand what it was testing so I presumed that it was a fragment of a piece of incomplete work. Removed on purpose. I may be mistaken about its value.

Since it extends ShareConsumerRackAwareTest, its purpose is to run testShareConsumerWithRackAwareAssignor without assignment batching.

It needs a comment 😄 I can reinstate it in the next commit, along with a comment.

chia7712 · 2026-04-14T06:52:52Z

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java

+            producer.flush();
+
+            // The acknowledgement commit callback will try to call a method of ShareConsumer
+            shareConsumer.setAcknowledgementCommitCallback(new TestableAcknowledgementCommitCallbackWakeup<>(shareConsumer));


shareConsumer.setAcknowledgementCommitCallback((__, ___) -> shareConsumer.wakeup());

and we can remove TestableAcknowledgementCommitCallbackWakeup

chia7712 · 2026-04-14T06:54:47Z

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java

+            producer.flush();
+
+            AtomicBoolean callbackCalled = new AtomicBoolean(false);
+            shareConsumer.setAcknowledgementCommitCallback(new TestableAcknowledgementCommitCallbackThrows(callbackCalled));


shareConsumer.setAcknowledgementCommitCallback((__, ___) -> { callbackCalled.set(true); throw new OutOfOrderSequenceException("Exception thrown in TestableAcknowledgementCommitCallbackThrows.onComplete"); });

and we could remove TestableAcknowledgementCommitCallbackThrows

MINOR: Split up ShareConsumerTest

7f1502a

github-actions bot added tests Test fixes (including flaky tests) clients labels Apr 9, 2026

nileshkumar3 reviewed Apr 10, 2026

View reviewed changes

...gration-tests/src/test/java/org/apache/kafka/clients/consumer/ShareConsumerCallbackTest.java Outdated Show resolved Hide resolved

AndrewJSchofield added the KIP-932 Queues for Kafka label Apr 10, 2026

Split out lag tests too

9c59431

see-quick reviewed Apr 10, 2026

View reviewed changes

Integration test tag added

e902a8d

chia7712 reviewed Apr 10, 2026

View reviewed changes

Review comments

f7b11fd

see-quick approved these changes Apr 12, 2026

View reviewed changes

chia7712 reviewed Apr 14, 2026

View reviewed changes

AndrewJSchofield added 2 commits April 14, 2026 22:26

More review comments

3220cd3

Merge branch 'trunk' into share-consumer-test-split

be24900

Conversation

AndrewJSchofield commented Apr 9, 2026 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

see-quick left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Apr 12, 2026

Uh oh!

see-quick commented Apr 13, 2026

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndrewJSchofield commented Apr 9, 2026 •

edited by github-actions bot

Loading