MINOR: Migrate EligibleLeaderReplicasIntegrationTest to use new test infra #20199

jim0987795064 · 2025-07-18T21:22:35Z

Changes: Use ClusterTest to rewrite EligibleLeaderReplicasIntegrationTest.

… infra

…ScalaBuffer with CollectionConverters.asScala

…ibleLeaderReplicasIntegrationTest

…import-control-server.xml, and fix the bug in restartDeadBrokers

chia7712

@jim0987795064 thanks for this patch. a couple of comments remain. PTAL

chia7712 · 2025-07-24T16:48:30Z

checkstyle/import-control-server.xml

-
-  <!-- no one depends on the server -->
-  <disallow pkg="kafka" />
+  <allow pkg="scala" />


Please use var to avoid importing those dependencies explicitly

chia7712 · 2025-07-24T16:50:00Z

server/src/test/java/org/apache/kafka/server/EligibleLeaderReplicasIntegrationTest.java

-            killBroker(initialReplicas.get(0).id());
-            killBroker(initialReplicas.get(1).id());
+            clusterInstance.shutdownBroker(initialReplicas.get(0).id());
+            clusterInstance.shutdownBroker(initialReplicas.get(1).id());

            waitForIsrAndElr((isrSize, elrSize) -> {


waitForIsrAndElr((isrSize, elrSize) -> isrSize == 0 && elrSize == 3);

chia7712 · 2025-07-24T16:50:11Z

server/src/test/java/org/apache/kafka/server/EligibleLeaderReplicasIntegrationTest.java

-                return b.config().brokerId() == brokerToBeUncleanShutdown;
-            }).get();
+            KafkaBroker broker = clusterInstance.brokers().values().stream().filter(b -> b.config().brokerId() == brokerToBeUncleanShutdown)
+                .findFirst().get();
            Seq<File> dirs = broker.logManager().liveLogDirs();
            assertEquals(1, dirs.size());
            CleanShutdownFileHandler handler = new CleanShutdownFileHandler(dirs.apply(0).toString());
            assertTrue(handler.exists());
            assertDoesNotThrow(() -> handler.delete());


assertDoesNotThrow(handler::delete);

chia7712 · 2025-07-24T16:52:08Z

test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/ClusterInstance.java

+            throw new IllegalArgumentException("Must supply at least one server config.");
+        }
+        brokers().entrySet().forEach(entry -> {
+            if (!entry.getValue().isShutdown()) {


it seems the method is used to restart "dead" brokers, so the condition should be if (entry.getValue().isShutdown()), right?

Thanks for the feedback. I've addressed issues mentioned above.

chia7712 · 2025-08-05T13:28:34Z

@jim0987795064 could you please fix the conflicts

jim0987795064 · 2025-08-06T11:29:27Z

@jim0987795064 could you please fix the conflicts

Thanks for this comment, I've already fixed this conflicts

…tionTest

chia7712

@jim0987795064 thanks for this patch!

chia7712 · 2025-08-07T18:46:18Z

server/src/test/java/org/apache/kafka/server/EligibleLeaderReplicasIntegrationTest.java

-            waitForIsrAndElr((isrSize, elrSize) -> {
-                return isrSize == 2 && elrSize == 1;
-            });
+            waitForIsrAndElr((isrSize, elrSize) -> isrSize == 2 && elrSize == 1);

            // Now the partition is under min ISR. HWM should not advance.
            producer.send(new ProducerRecord<>(testTopicName, "1", "1")).get();
            Thread.sleep(100);


Please use TimeUnit instead

chia7712 · 2025-08-07T18:46:40Z

test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/ClusterInstance.java

@@ -447,4 +447,15 @@ default int waitUntilLeaderIsElectedOrChangedWithAdmin(Admin admin,
        throw new AssertionError("Timing out after " + timeoutMs +
                " ms since a leader was not elected for partition " + topicPartition);
    }
+
+    default void restartDeadBrokers() throws ExecutionException {


ExecutionException is unnecessary

chia7712 · 2025-08-07T18:46:53Z

test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/ClusterInstance.java

+        if (brokers().isEmpty()) {
+            throw new RuntimeException("Must supply at least one server config.");
+        }
+        brokers().entrySet().forEach(entry -> {


brokers().forEach((key, value) -> { if (value.isShutdown()) value.startup(); });

chia7712 · 2025-08-07T18:48:59Z

server/src/test/java/org/apache/kafka/server/EligibleLeaderReplicasIntegrationTest.java

            );
        } finally {
-            restartDeadBrokers(false);
+            clusterInstance.restartDeadBrokers();


This helper method is very simple, so perhaps we could move it into the test class

@chia7712
Thanks for the suggestions. I've addressed problems mentioned above.

…licasIntegrationTest

…write-EligibleLeaderReplicasIntegrationTest

refactor: rewrite EligibleLeaderReplicasIntegrationTest with new test…

0cf74cd

… infra

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

jim0987795064 added 9 commits July 21, 2025 21:06

rewrite the logic of ClusterTestDefaults and restartDeadBrokers

9dd08e5

Rewrite the logic of restartDeadBrokers and replace JavaConverters.as…

9aa594e

…ScalaBuffer with CollectionConverters.asScala

Merge remote-tracking branch 'upstream/trunk' into MINOR-rewrite-Elig…

7a1c937

…ibleLeaderReplicasIntegrationTest

Merge remote-tracking branch 'upstream/trunk' into MINOR-rewrite-Elig…

b705af2

…ibleLeaderReplicasIntegrationTest

Merge remote-tracking branch 'upstream/trunk' into MINOR-rewrite-Elig…

fe136a7

…ibleLeaderReplicasIntegrationTest

Move EligibleLeaderReplicasIntegrationTest to server module, rewrite …

eb25348

…import-control-server.xml, and fix the bug in restartDeadBrokers

Fix checkstyleTest error

f3d61ef

Add metadataVersion to ClusterTest

e68ee42

replace KafkaException with InterruptedException

4fd3323

chia7712 reviewed Jul 24, 2025

View reviewed changes

refacotr: Replace Scala Seq with Java List

493a7c7

jim0987795064 changed the title ~~(WIP) MINOR: Migrate EligibleLeaderReplicasIntegrationTestto use new test infra~~ MINOR: Migrate EligibleLeaderReplicasIntegrationTestto use new test infra Jul 24, 2025

github-actions bot removed the triage PRs from the community label Jul 25, 2025

chia7712 changed the title ~~MINOR: Migrate EligibleLeaderReplicasIntegrationTestto use new test infra~~ MINOR: Migrate EligibleLeaderReplicasIntegrationTest to use new test infra Jul 30, 2025

jim0987795064 force-pushed the MINOR-rewrite-EligibleLeaderReplicasIntegrationTest branch 2 times, most recently from 0d7bcf8 to 493a7c7 Compare August 6, 2025 13:21

jim0987795064 added 2 commits August 6, 2025 15:44

Merge branch 'trunk' into MINOR-rewrite-EligibleLeaderReplicasIntegra…

43402e1

…tionTest

refactor: Replace RuntimeException with IllegalArgumentException

4c17b97

chia7712 added the ci-approved label Aug 7, 2025

chia7712 reviewed Aug 7, 2025

View reviewed changes

jim0987795064 force-pushed the MINOR-rewrite-EligibleLeaderReplicasIntegrationTest branch from 09a8771 to 4c17b97 Compare August 7, 2025 19:31

jim0987795064 added 2 commits August 7, 2025 21:32

refacor: Refactor restartDeadBrokers and move it to EligibleLeaderRep…

927bf65

…licasIntegrationTest

Merge branch 'trunk' of https://github.com/apache/kafka into MINOR-re…

fa607e2

…write-EligibleLeaderReplicasIntegrationTest

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MINOR: Migrate EligibleLeaderReplicasIntegrationTest to use new test infra #20199

MINOR: Migrate EligibleLeaderReplicasIntegrationTest to use new test infra #20199

jim0987795064 commented Jul 18, 2025 •

edited

Loading

Uh oh!

chia7712 left a comment

Uh oh!

chia7712 Jul 24, 2025

Uh oh!

chia7712 Jul 24, 2025

Uh oh!

chia7712 Jul 24, 2025

Uh oh!

chia7712 Jul 24, 2025

Uh oh!

jim0987795064 Jul 24, 2025

Uh oh!

chia7712 commented Aug 5, 2025

Uh oh!

jim0987795064 commented Aug 6, 2025

Uh oh!

chia7712 left a comment

Uh oh!

chia7712 Aug 7, 2025

Uh oh!

chia7712 Aug 7, 2025

Uh oh!

chia7712 Aug 7, 2025

Uh oh!

chia7712 Aug 7, 2025

Uh oh!

jim0987795064 Aug 7, 2025

Uh oh!

Uh oh!

MINOR: Migrate EligibleLeaderReplicasIntegrationTest to use new test infra #20199

Are you sure you want to change the base?

MINOR: Migrate EligibleLeaderReplicasIntegrationTest to use new test infra #20199

Conversation

jim0987795064 commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chia7712 commented Aug 5, 2025

Uh oh!

jim0987795064 commented Aug 6, 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!

Uh oh!

jim0987795064 commented Jul 18, 2025 •

edited

Loading