KAFKA-18915: Rewrite AdminClientRebootstrapTest to cover the current scenario #19187

clarkwtc · 2025-03-11T16:07:59Z

Follow-up: #19094

We need to rewrite AdminClientRebootstrapTest to cover the current scenario.
Added the admin client with rebootstrap disabled, as the admin client uses the default AdminClientRebootstrap configuration setting.

Default AdminClientRebootstrap config:

properties.put(CommonClientConfigs.METADATA_RECOVERY_STRATEGY_CONFIG, "rebootstrap");
properties.put(CommonClientConfigs.METADATA_RECOVERY_REBOOTSTRAP_TRIGGER_MS_CONFIG, "3000000");
properties.put(CommonClientConfigs.SOCKET_CONNECTION_SETUP_TIMEOUT_MS_CONFIG, "10000");
properties.put(CommonClientConfigs.SOCKET_CONNECTION_SETUP_TIMEOUT_MAX_MS_CONFIG, "30000");
properties.put(CommonClientConfigs.RECONNECT_BACKOFF_MS_CONFIG, "50L");
properties.put(CommonClientConfigs.RECONNECT_BACKOFF_MAX_MS_CONFIG, "1000L");

The test case for the admin client with enabled rebootstrap

The test case for the admin client with disabled rebootstrap

Reviewers: Jhen-Yung Hsu jhenyunghsu@gmail.com, TengYao Chi kitingiao@gmail.com, Ken Huang s7133700@gmail.com, Chia-Ping Tsai chia7712@gmail.com

frankvicky

@clarkwtc : Thanks for the patch.

frankvicky · 2025-03-12T08:30:50Z

core/src/test/java/kafka/test/api/AdminClientRebootstrapTest.java

-            // Only the server 0 is available for the admin client during the bootstrap.
-            TestUtils.waitForCondition(() -> admin.listTopics().names().get(timeout, TimeUnit.MINUTES).contains(topic),
+            // Only the broker 1 is available for the admin client during the bootstrap.
+            TestUtils.waitForCondition(() -> admin.listTopics().names().get(timeout, TimeUnit.MINUTES).contains(TOPIC),


Out of curiosity, why did we have a 5-minute timeout here?

Thank you for your comments.
Maybe it’s a bit too long to wait here.
I updated to 60 seconds.

Yunyung · 2025-03-12T15:35:08Z

core/src/test/java/kafka/test/api/AdminClientRebootstrapTest.java

+        clusterInstance.startBroker(broker0);
+
+        // The broker 1, originally cached during the bootstrap, is offline.
+        // The admin client will throw a TimeoutException because the brokers are offline during the bootstrap list


This code comment is not correct.

Thank you for your comments.
Sure, this situation refers to the brokers being offline since they were cached during the bootstrap.
I updated the code comments.

chia7712 · 2025-03-12T15:39:57Z

@clarkwtc cloud you please move this test to clients-integration-tests module?

Yunyung · 2025-03-12T15:46:14Z

core/src/test/java/kafka/test/api/AdminClientRebootstrapTest.java

+        // The broker 1, originally cached during the bootstrap, is offline.
+        // The admin client will throw a TimeoutException because the brokers are offline during the bootstrap list
+        assertThrows(TimeoutException.class, () -> admin.listTopics().names().get(5, TimeUnit.SECONDS));
+        // The admin client needs to wait the default timeout for other threads because the brokers are offline.


Yunyung · 2025-03-12T15:46:25Z

core/src/test/java/kafka/test/api/AdminClientRebootstrapTest.java

+        // The admin client will throw a TimeoutException because the brokers are offline during the bootstrap list
+        assertThrows(TimeoutException.class, () -> admin.listTopics().names().get(5, TimeUnit.SECONDS));
+        // The admin client needs to wait the default timeout for other threads because the brokers are offline.
+        admin.close(Duration.ofSeconds(0));


nit: Duration.ZERO

I've fixed it.

…_test-testing-scenario

clarkwtc · 2025-03-12T17:07:06Z

@chia7712
I already moved this test to clients-integration-tests.

chia7712 · 2025-03-12T17:50:44Z

@clarkwtc please check the build error

…_test-testing-scenario

clarkwtc · 2025-03-13T02:08:00Z

@chia7712
Sorry, I missed it.
I've fixed it.

chia7712

@clarkwtc thanks for your patch.

chia7712 · 2025-03-13T05:15:31Z

...tegration-tests/src/test/java/org/apache/kafka/clients/admin/AdminClientRebootstrapTest.java

+    private static final int PARTITIONS = 2;
+
+    @ClusterTest(
+        brokers = 2,


2 -> PARTITIONS

Thank you for your comments.
I've fixed it.

chia7712 · 2025-03-13T05:15:35Z

...tegration-tests/src/test/java/org/apache/kafka/clients/admin/AdminClientRebootstrapTest.java

+    }
+
+    @ClusterTest(
+        brokers = 2,


I've fixed it.

chia7712 · 2025-03-13T05:16:44Z

...tegration-tests/src/test/java/org/apache/kafka/clients/admin/AdminClientRebootstrapTest.java

+        brokers = 2,
+        types = {Type.KRAFT},
+        serverProperties = {
+            @ClusterConfigProperty(key = TopicConfig.UNCLEAN_LEADER_ELECTION_ENABLE_CONFIG, value = "true"),


we don't need those configs, since this test case does not use either producer or consumer.

Sorry, I wrote the config for ProducerClientRebootstrapTest, but I messed up how to use it.

chia7712 · 2025-03-13T05:16:52Z

...tegration-tests/src/test/java/org/apache/kafka/clients/admin/AdminClientRebootstrapTest.java

+        brokers = 2,
+        types = {Type.KRAFT},
+        serverProperties = {
+            @ClusterConfigProperty(key = TopicConfig.UNCLEAN_LEADER_ELECTION_ENABLE_CONFIG, value = "true"),


I've fixed it.

chia7712 · 2025-03-13T05:17:32Z

...tegration-tests/src/test/java/org/apache/kafka/clients/admin/AdminClientRebootstrapTest.java

+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class AdminClientRebootstrapTest {


maybe we can rename it to ClientRebootstrapTest and merge producer/consumer into this class

No problem;
I also changed ClientRebootstrapTest file location from org/apache/kafka/clients/admin to org/apache/kafka/clients and renamed functions.

m1a2st

Thanks for this patch, a little comment

m1a2st · 2025-03-13T07:45:53Z

...tegration-tests/src/test/java/org/apache/kafka/clients/admin/AdminClientRebootstrapTest.java

+            admin.createTopics(List.of(new NewTopic(TOPIC, PARTITIONS, (short) 2)));
+
+            // Only the broker 1 is available for the admin client during the bootstrap.
+            Assertions.assertDoesNotThrow(() -> admin.listTopics().names().get(timeout, TimeUnit.SECONDS).contains(TOPIC));


nit: static import

Thank you for your comments.
I've fixed it.

- Change Assertions to static import - Redefine AdminClientRebootstrapTest to ClientRebootstrapTest - Remove unused config UNCLEAN_LEADER_ELECTION_ENABLE_CONFIG

…_test-testing-scenario

chia7712

@clarkwtc thanks for this patch.

chia7712 · 2025-03-14T20:31:52Z

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

+
+public class ClientRebootstrapTest {
+    private static final String TOPIC = "topic";
+    private static final int PARTITIONS = 2;


the purpose is to ensure each broker has a replica, and hence it should be renamed to "REPLICAS"

I've fixed it.

chia7712 · 2025-03-14T20:32:24Z

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

+        clusterInstance.shutdownBroker(broker0);
+
+        try (var admin = clusterInstance.admin()) {
+            admin.createTopics(List.of(new NewTopic(TOPIC, PARTITIONS, (short) 2)));


All we need is "one partition" and "two replicas", right?

Sure, we only need one partition across two brokers because the topic will be replicated to both brokers.

…_test-testing-scenario

feat: Rewrite AdminClientRebootstrapTest to cover the current scenario

6c47aee

github-actions bot added triage PRs from the community core Kafka Broker tests Test fixes (including flaky tests) labels Mar 11, 2025

chia7712 added the ci-approved label Mar 11, 2025

clarkwtc mentioned this pull request Mar 11, 2025

KAFKA-18915: Migrate AdminClientRebootstrapTest to use new test infra #19094

Merged

frankvicky reviewed Mar 12, 2025

View reviewed changes

Yunyung reviewed Mar 12, 2025

View reviewed changes

clarkwtc added 3 commits March 13, 2025 00:47

chore: Make it better.

8193ac8

Merge branch 'trunk' into KAFKA-18915-rewrite-adminclient_rebootstrap…

d1d1bfb

…_test-testing-scenario

feat: Move this test to clients-integration-tests

74413e4

github-actions bot added the clients label Mar 12, 2025

clarkwtc added 2 commits March 13, 2025 09:39

fix: Check style

671273d

Merge branch 'trunk' into KAFKA-18915-rewrite-adminclient_rebootstrap…

3dc687d

…_test-testing-scenario

github-actions bot removed the triage PRs from the community label Mar 13, 2025

chia7712 reviewed Mar 13, 2025

View reviewed changes

m1a2st reviewed Mar 13, 2025

View reviewed changes

clarkwtc added 2 commits March 14, 2025 01:47

chore: Make it better

393c08f

- Change Assertions to static import - Redefine AdminClientRebootstrapTest to ClientRebootstrapTest - Remove unused config UNCLEAN_LEADER_ELECTION_ENABLE_CONFIG

Merge branch 'trunk' into KAFKA-18915-rewrite-adminclient_rebootstrap…

fa424bd

…_test-testing-scenario

chia7712 reviewed Mar 14, 2025

View reviewed changes

clarkwtc added 2 commits March 15, 2025 20:16

fix: Redefine test case to use only one partitions.

b5de129

Merge branch 'trunk' into KAFKA-18915-rewrite-adminclient_rebootstrap…

9220e60

…_test-testing-scenario

chia7712 approved these changes Mar 15, 2025

View reviewed changes

chia7712 merged commit e05b0e6 into apache:trunk Mar 15, 2025
22 checks passed


		import static org.junit.jupiter.api.Assertions.assertThrows;

		public class AdminClientRebootstrapTest {

KAFKA-18915: Rewrite AdminClientRebootstrapTest to cover the current scenario #19187

KAFKA-18915: Rewrite AdminClientRebootstrapTest to cover the current scenario #19187

Uh oh!

Conversation

clarkwtc commented Mar 11, 2025 • edited by chia7712 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankvicky 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!

Yunyung Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Mar 12, 2025

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!

clarkwtc commented Mar 12, 2025

Uh oh!

chia7712 commented Mar 12, 2025

Uh oh!

clarkwtc commented Mar 13, 2025

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!

clarkwtc Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st 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!

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!

Uh oh!

Reviewers

Assignees

clarkwtc commented Mar 11, 2025 •

edited by chia7712

Loading

Yunyung Mar 12, 2025 •

edited

Loading

clarkwtc Mar 13, 2025 •

edited

Loading