KAFKA-18914 Migrate ConsumerRebootstrapTest to use new test infra #19154

clarkwtc · 2025-03-07T15:01:18Z

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

The PR changed three things.

Migrated ConsumerRebootstrapTest to new test infra and removed the old Scala test.
Updated the original test case to cover rebootstrap scenarios.
Integrated ConsumerRebootstrapTest into ClientRebootstrapTest in the client-integration-tests module.
Removed the RebootstrapTest.scala.

Default ConsumerRebootstrap config:

properties.put(CommonClientConfigs.METADATA_RECOVERY_STRATEGY_CONFIG, "rebootstrap");
properties.put(CommonClientConfigs.METADATA_RECOVERY_REBOOTSTRAP_TRIGGER_MS_CONFIG, "300000");
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 consumer with enabled rebootstrap

The test case for the consumer with disabled rebootstrap

Reviewers: Chia-Ping Tsai chia7712@gmail.com

github-actions · 2025-03-15T03:13:09Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

chia7712 · 2025-03-18T06:52:07Z

@clarkwtc could you please merge it into ClientRebootstrapTest?

clarkwtc · 2025-03-19T02:26:25Z

@chia7712
I merged it into ClientRebootstrapTest.

github-actions · 2025-03-20T03:15:43Z

A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the triage label
should be removed to prevent this automation from happening again.

chia7712

@clarkwtc thanks for this patch.

chia7712 · 2025-03-20T16:08:29Z

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

+            try (var producer = clusterInstance.producer()) {
+                var recordMetadata = producer.send(new ProducerRecord<>(TOPIC, part, "key 1".getBytes(), "value 1".getBytes())).get();
+                assertEquals(1, recordMetadata.offset());
+                producer.flush();


we don't need to call flush after you call the get

Maybe we should remove to call get?
I need to call flush; without it, if the broker restarts on rebootstrap, the consumer polls nothing.

Given that acks is not set to 0, the get method guarantees that the record is transmitted to the server. This behavior is equivalent to a flush operation.

Sure, set acks=-1 to guarantee that the record will not be lost.

chia7712 · 2025-03-20T16:09:01Z

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

+        var tp = new TopicPartition(TOPIC, part);
+
+        try (var producer = clusterInstance.producer()) {
+            var recordMetadata = producer.send(new ProducerRecord<>(TOPIC, part, "key 0".getBytes(), "value 0".getBytes())).get();


there is only one partition, so we can streamline the code:

var recordMetadata = producer.send(new ProducerRecord<>(TOPIC, "value 0".getBytes())).get();

Got it; make it simple.

chia7712 · 2025-03-20T16:09:06Z

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

+            clusterInstance.startBroker(broker0);
+
+            try (var producer = clusterInstance.producer()) {
+                var recordMetadata = producer.send(new ProducerRecord<>(TOPIC, part, "key 1".getBytes(), "value 1".getBytes())).get();


I fixed it.

chia7712 · 2025-03-20T16:24:38Z

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

+            // Only the server 1 is available for the consumer during the bootstrap.
+            consumer.assign(partitions);
+            consumer.seekToBeginning(partitions);
+            assertEquals(1, consumer.poll(Duration.ofSeconds(1)).count());


we can't assume the records get returned by only one poll. could you please add loop to avoid flaky?

Okay, I use TestUtils.waitForCondition to prevent flaky.

chia7712 · 2025-03-20T16:24:41Z

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

+
+            // The server 1 originally cached during the bootstrap, is offline.
+            // However, the server 0 from the bootstrap list is online.
+            assertEquals(1, consumer.poll(Duration.ofSeconds(1)).count());


I fixed it.

chia7712 · 2025-03-20T16:24:50Z

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

+        var partitions = List.of(new TopicPartition(TOPIC, part));
+
+        try (var producer = clusterInstance.producer()) {
+            var recordMetadata = producer.send(new ProducerRecord<>(TOPIC, part, "key 0".getBytes(), "value 0".getBytes())).get();


I fixed it.

- Add ack equal 1 writing the record to local log. - Wait consumer to poll record, that avoid flaky. - Streamline ProducerRecord because only one partition.

…o acknowledge the record.

…ache#19154) Migrate ConsumerRebootstrapTest to the new test infra and remove the old Scala test. The PR changed three things. * Migrated `ConsumerRebootstrapTest` to new test infra and removed the old Scala test. * Updated the original test case to cover rebootstrap scenarios. * Integrated `ConsumerRebootstrapTest` into `ClientRebootstrapTest` in the `client-integration-tests` module. * Removed the `RebootstrapTest.scala`. Default `ConsumerRebootstrap` config: > properties.put(CommonClientConfigs.METADATA_RECOVERY_STRATEGY_CONFIG, "rebootstrap"); properties.put(CommonClientConfigs.METADATA_RECOVERY_REBOOTSTRAP_TRIGGER_MS_CONFIG, "300000"); 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 consumer with enabled rebootstrap ![Screenshot 2025-03-22 at 9 48 13 PM](https://github.com/user-attachments/assets/8470549f-a24c-43fa-ae44-789cbf422a63) The test case for the consumer with disabled rebootstrap ![Screenshot 2025-03-22 at 9 47 22 PM](https://github.com/user-attachments/assets/0a183464-6a74-449f-8e71-d641a6ea5bb1) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

clarkwtc added 2 commits March 7, 2025 22:50

feat: Migrate ConsumerRebootstrapTest to use new test infra

f4abfd3

feat: Remove old ConsumerRebootstrapTest test

a40bc46

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

chia7712 added the ci-approved label Mar 9, 2025

github-actions bot added the needs-attention label Mar 15, 2025

Merge branch 'trunk' into KAFKA-18914-rewrite-consumer_rebootstrap_test

3d3c3d1

clarkwtc added 2 commits March 19, 2025 10:19

Merge branch 'trunk' into KAFKA-18914-rewrite-consumer_rebootstrap_test

36b0d51

feat: Move this test to clients-integration-tests

2d190bb

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

github-actions bot removed the needs-attention label Mar 19, 2025

feat: Cover rebootstrap of classic consumer test cases.

ecb5720

github-actions bot added the needs-attention label Mar 20, 2025

chia7712 reviewed Mar 20, 2025

View reviewed changes

github-actions bot removed needs-attention triage PRs from the community labels Mar 21, 2025

clarkwtc added 3 commits March 22, 2025 20:55

fix: Make it be better

44edfc8

- Add ack equal 1 writing the record to local log. - Wait consumer to poll record, that avoid flaky. - Streamline ProducerRecord because only one partition.

Merge branch 'trunk' into KAFKA-18914-rewrite-consumer_rebootstrap_test

2b5508c

fix: Set acks=all for the leader wait setting of in-sync replicas t…

f84d68a

…o acknowledge the record.

clarkwtc changed the title ~~Kafka-18914 Migrate ConsumerRebootstrapTest to use new test infra~~ KAFKA-18914 Migrate ConsumerRebootstrapTest to use new test infra Mar 22, 2025

clarkwtc added 4 commits March 23, 2025 12:30

chore: Streamline variable part in TopicPartition.

e5f5797

Merge branch 'trunk'

fa7a768

feat: Remove unused RebootstrapTest.scala

41590bf

chore: Set PARTITIONS to global variable in ClientRebootstrapTest

9cc420d

chia7712 approved these changes Mar 25, 2025

View reviewed changes

chia7712 merged commit 1547204 into apache:trunk Mar 25, 2025
24 checks passed

KAFKA-18914 Migrate ConsumerRebootstrapTest to use new test infra #19154

KAFKA-18914 Migrate ConsumerRebootstrapTest to use new test infra #19154

Uh oh!

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Mar 15, 2025

Uh oh!

chia7712 commented Mar 18, 2025

Uh oh!

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

Uh oh!

Uh oh!

github-actions bot commented Mar 20, 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!

clarkwtc Mar 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarkwtc Mar 22, 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clarkwtc commented Mar 7, 2025 •

edited by chia7712

Loading

clarkwtc commented Mar 19, 2025 •

edited

Loading

clarkwtc Mar 21, 2025 •

edited

Loading

clarkwtc Mar 22, 2025 •

edited

Loading