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

clarkwtc · 2025-03-04T14:46:09Z

Migrate AdminClientRebootstrapTest to the new test infra and remove the old Scala test.

The test results

Reviewers: TengYao Chi kitingiao@gmail.com, David Arthur mumrah@gmail.com

frankvicky

Overall LGTM.
I have a nit comment.

frankvicky · 2025-03-04T14:52:41Z

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

+        var topic = "topic";
+        try (var admin = clusterInstance.admin()) {
+            admin.createTopics(List.of(new NewTopic(topic, BROKER_COUNT, (short) 2)));


nit: inline topic

mumrah · 2025-03-04T15:40:49Z

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

+                .setTypes(Set.of(Type.KRAFT))
+                .setBrokers(BROKER_COUNT)
+                .setAdminClientProperties(rebootstrapProperties)
+                .setServerProperties(serverProperties).build();


nit: here and above there's too much whitespace

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

mumrah · 2025-03-04T15:41:59Z

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

+            server1.awaitShutdown();
+
+            // Only the server 0 is available for the admin client during the bootstrap.
+            admin.listTopics().names().get();


I know you've just translated an existing test, but it's good to use timeouts with these get() calls.

Sure, adding timeouts to prevent get() calls from blocking.

…_test

mumrah · 2025-03-04T18:58:07Z

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


            // Only the server 0 is available for the admin client during the bootstrap.
-            admin.listTopics().names().get();
+            TestUtils.waitForCondition(() -> admin.listTopics().names().get().contains(topic),


This get() will still block until the future completes. What I was suggestion was that we do something like .get(5, TimeUnit.MINUTES) or some reasonable timeout. This would prevent the test from getting stuck if something went wrong.

Adding the waitForCondition is a good improvement. It will make this test more reliable.

I see. Adding the waitForCondition only waits for get() to complete, your suggestion prevents the testing blocking.
I've fixed it.

…_test

mumrah

LGTM Thanks!

chia7712

@clarkwtc sorry for late review. I leave some major comments. Please take a look, thanks!

chia7712 · 2025-03-09T03:29:08Z

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

+        return ClusterConfig.defaultBuilder()
+            .setTypes(Set.of(Type.KRAFT))
+            .setBrokers(BROKER_COUNT)
+            .setAdminClientProperties(rebootstrapProperties)


this method is no-op, and we should use clusterInstance.admin(Map) to build admin with custom configs.

open https://issues.apache.org/jira/browse/KAFKA-18944 to remove those unused setters

chia7712 · 2025-03-09T03:30:11Z

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

+        var topic = "topic";
+        var timeout = 5;
+        try (var admin = clusterInstance.admin()) {
+            admin.createTopics(List.of(new NewTopic(topic, BROKER_COUNT, (short) 2)));


BROKER_COUNT should be used to define replicas rather than partitions, since our goal is to ensure each broker has a replica.

Sure, these are different definitions and should not reuse variables in partitions.

chia7712 · 2025-03-09T03:36:43Z

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

+    }
+
+    @ClusterTemplate(value = "generator")
+    public void testRebootstrap(ClusterInstance clusterInstance) throws InterruptedException {


After rewriting, this test no longer covers the scenario. We need to verify that the admin can "rebootstrap" when all "known" brokers are unavailable. Therefore, we should shut down one broker before creating the admin to ensure it's aware of "one" broker. Then, we shut down the broker the admin is aware of and restart the other one.

Shut down broker0.

Create the admin with bootstrap=broker0,broker1.

The admin is aware of broker1.

Run some admin APIs to ensure everything is fine.

Shut down broker1 and restart broker0.

The admin can't connect to broker1, so it starts to rebootstrap to find other available brokers.

The admin finds broker0.

Run some admin APIs to ensure everything is fine.

Thank you for redefining this test to cover the scenario.
We will move forward with this PR: #19187

Remove unused `saslServerProperties`, `saslClientProperties`, `adminClientProperties`, `producerProperties`, and `consumerProperties` in ClusterConfig. First, I quickly fixed the unused adminClientProperties, and then I will move on to #19094 to fix the related issues. Pass AdminClientRebootstrapTest <img width="1398" alt="Screenshot 2025-03-09 at 12 54 57 PM" src="https://github.com/user-attachments/assets/73c50376-6602-493d-8abd-0eb2bb304114" /> Pass ClusterConfigTest <img width="1117" alt="Screenshot 2025-03-09 at 12 55 28 PM" src="https://github.com/user-attachments/assets/b4da59da-dfdf-4698-9077-5086854360ab" /> Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

clarkwtc added 2 commits March 4, 2025 22:31

feat: Migrate AdminClientRebootstrapTest to use new test infra

5645bb7

feat: Remove old AdminClientRebootstrapTest test

c6f2c76

github-actions bot added triage PRs from the community core Kafka Broker labels Mar 4, 2025

chia7712 added the ci-approved label Mar 4, 2025

frankvicky reviewed Mar 4, 2025

View reviewed changes

Fix: Inline topic variable

9df423d

mumrah reviewed Mar 4, 2025

View reviewed changes

clarkwtc added 4 commits March 4, 2025 23:42

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

89db960

…_test

Fix: Remove unnecessary whitespace

045addc

Fix: Add timeout to avoid waiting too long

ba8ddf4

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

fe15785

…_test

mumrah reviewed Mar 4, 2025

View reviewed changes

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

clarkwtc added 2 commits March 6, 2025 20:52

Fix: Add timeout to prevent blocking until the future completes

43311ca

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

f1b6411

…_test

mumrah mentioned this pull request Mar 6, 2025

KAFKA-18933 Add client integration tests module #19144

Merged

mumrah approved these changes Mar 6, 2025

View reviewed changes

mumrah merged commit 870db5d into apache:trunk Mar 6, 2025
24 checks passed

chia7712 reviewed Mar 9, 2025

View reviewed changes

chia7712 mentioned this pull request Mar 9, 2025

KAFKA-18276 Migrate ProducerRebootstrapTest to new test infra #19046

Merged

clarkwtc mentioned this pull request Mar 9, 2025

KAFKA-18944: Remove unused setters from ClusterConfig #19166

Merged

clarkwtc mentioned this pull request Mar 11, 2025

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

Merged

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

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

Uh oh!

Conversation

clarkwtc commented Mar 4, 2025 • edited by mumrah 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!

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!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

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

clarkwtc commented Mar 4, 2025 •

edited by mumrah

Loading