MINOR: add test for `kafka-consumer-groups.sh` should not fail when partition offline by TaiJuWu · Pull Request #20235 · apache/kafka

TaiJuWu · 2025-07-24T07:50:29Z

See: #20168 (comment)

add follow test case:

Given a topic with three partitions, where partition t-2 is offline,
if partitionsToReset contains only t-1, the method
filterNoneLeaderPartitions incorrectly includes t-2 in the result,
leading to a failure in the tool.

Reviewers: Chia-Ping Tsai chia7712@gmail.com, Jhen-Yung Hsu
jhenyunghsu@gmail.com, Ken Huang s7133700@gmail.com, Andrew
Schofield aschofield@confluent.io

This reverts commit a0fdb86.

AndrewJSchofield

Thanks for the PR. A couple of small comments.

AndrewJSchofield · 2025-07-24T07:57:41Z

+        cluster.createTopic(topic, 3, (short) 2);
+
+        try (ConsumerGroupCommand.ConsumerGroupService service = getConsumerGroupService(args);
+             Admin admin = cluster.admin()) {


nit: The indentation here is a bit strange not being a multiple of 4 spaces. I wonder whether just concatenating this on the end of the previous line would be simplest.

change to
try (Admin admin = cluster.admin(); ConsumerGroupCommand.ConsumerGroupService service = getConsumerGroupService(args))

Yunyung · 2025-07-24T11:35:36Z

        }
    }

+    @ClusterTest(brokers = 5)


I think we could just use 2 brokers for this test? 5 is overkill.

Thanks for review.
I use 3 broker as default since we need to ensure __consumere_offset always have two brokers in any condition and we need to shutdown a broker.

If the replica of __consumer_offset is 1, we can't sure the leader is not shutdown broker.

Can two brokers with two replicas fix the issue you mentioned?

Change to two brokers and explicitly define min_insyn_replica equals one.

Yunyung · 2025-07-24T12:04:54Z

    }

+    @ClusterTest(brokers = 5)
+    public void testResetOffsetsWithOfflinePartition(ClusterInstance cluster) throws Exception {


To better distinguish this test from testResetOffsetsWithPartitionNoneLeader, maybe rename it to testResetOffsetsWithOfflinePartitionNotInResetTarget or something similar.

Yunyung

Thanks for the updates. Left two comments, one here: https://github.com/apache/kafka/pull/20168/files#r2228628205

Yunyung · 2025-07-24T14:01:01Z

                        .stream()
                        .flatMap(entry -> entry.getValue().partitions().stream()
-                                .filter(partitionInfo -> partitionInfo.leader() == null)
+                                .filter(partitionInfo -> topicPartitions.contains(new TopicPartition(entry.getKey(), partitionInfo.partition())) && partitionInfo.leader() == null)


nit: check partitionInfo.leader() == null before creating the TopicPartition for performance.

Good point, thanks.

This reverts commit 05b504b.

Yunyung · 2025-07-24T14:35:59Z

+            cluster.waitTopicCreation(topic, 2);
+
+            cluster.shutdownBroker(1);
+            TestUtils.waitForCondition(() -> !cluster.aliveBrokers().containsKey(1),


According to https://github.com/apache/kafka/pull/20168/files#r2228677312, I understand that shutdown() already waits until the shutdown is complete. So this can be removed as well.

chia7712

@TaiJuWu thanks for this patch

chia7712 · 2025-07-24T16:05:42Z

                        .flatMap(entry -> entry.getValue().partitions().stream()
-                                .filter(partitionInfo -> partitionInfo.leader() == null)
+                                .filter(partitionInfo -> partitionInfo.leader() == null && topicPartitions.contains(new TopicPartition(entry.getKey(), partitionInfo.partition())))
                                .map(partitionInfo -> new TopicPartition(entry.getKey(), partitionInfo.partition())))


Please add the filter after map to avoid creating extra TopicPartition object.

.filter(partitionInfo -> partitionInfo.leader() == null) .map(partitionInfo -> new TopicPartition(entry.getKey(), partitionInfo.partition())) .filter(topicPartitions::contains))

chia7712 · 2025-07-24T16:06:39Z

    }

+    @ClusterTest(
+        brokers = 3,


It seems two brokers could work well for this test, right?

Here is the reason I selected three brokers instead of two.
#20235 (comment)

Yunyung · 2025-07-25T09:31:57Z

+            @ClusterConfigProperty(key = OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "2"),
+            @ClusterConfigProperty(key = TopicConfig.MIN_IN_SYNC_REPLICAS_CONFIG, value = "1")


Do we really need to set these properties? We can reproduce the failure without setting them.

These settings can avoid flaky.
In my mind, if __consumer_offset is at broker 1 and we shutdown it, we don't have any __consumer_offset.

The min.insync.replicas default value is 1, so we don't need to configure it explicitly?

chia7712 · 2025-07-30T11:59:01Z

@TaiJuWu please fix the build error

* What went wrong:
Execution failed for task ':tools:spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/org/apache/kafka/tools/consumer/group/ResetConsumerGroupOffsetTest.java
          @@ -27,7 +27,6 @@
           import·org.apache.kafka.clients.producer.ProducerRecord;
           import·org.apache.kafka.common.GroupState;
           import·org.apache.kafka.common.TopicPartition;
          -import·org.apache.kafka.common.config.TopicConfig;
           import·org.apache.kafka.common.errors.LeaderNotAvailableException;
           import·org.apache.kafka.common.errors.UnknownTopicOrPartitionException;
           import·org.apache.kafka.common.serialization.ByteArraySerializer;
  Run './gradlew :tools:spotlessApply' to fix these violations.

TaiJuWu · 2025-07-30T12:10:12Z

@TaiJuWu please fix the build error

* What went wrong:
Execution failed for task ':tools:spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/org/apache/kafka/tools/consumer/group/ResetConsumerGroupOffsetTest.java
          @@ -27,7 +27,6 @@
           import·org.apache.kafka.clients.producer.ProducerRecord;
           import·org.apache.kafka.common.GroupState;
           import·org.apache.kafka.common.TopicPartition;
          -import·org.apache.kafka.common.config.TopicConfig;
           import·org.apache.kafka.common.errors.LeaderNotAvailableException;
           import·org.apache.kafka.common.errors.UnknownTopicOrPartitionException;
           import·org.apache.kafka.common.serialization.ByteArraySerializer;
  Run './gradlew :tools:spotlessApply' to fix these violations.

Thanks for remaining me!

AndrewJSchofield

Thanks for the PR. Looks good to me.

…artition offline (apache#20235) See: apache#20168 (comment) add follow test case: Given a topic with three partitions, where partition `t-2` is offline, if partitionsToReset contains only `t-1`, the method filterNoneLeaderPartitions incorrectly includes `t-2` in the result, leading to a failure in the tool. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jhen-Yung Hsu <jhenyunghsu@gmail.com>, Ken Huang <s7133700@gmail.com>, Andrew Schofield <aschofield@confluent.io>

TaiJuWu added 5 commits July 24, 2025 14:04

init integrationTest

13c821c

use delete dir to create offline partition

a0fdb86

Revert "use delete dir to create offline partition"

f7a4b03

This reverts commit a0fdb86.

add integration test

a18d896

fix

c2a24c9

github-actions Bot added tools small Small PRs triage PRs from the community labels Jul 24, 2025

AndrewJSchofield self-requested a review July 24, 2025 07:51

TaiJuWu mentioned this pull request Jul 24, 2025

KAFKA-19500: kafka-consumer-groups.sh should fail quickly if the partition leader is unavailable #20168

Merged

AndrewJSchofield requested changes Jul 24, 2025

View reviewed changes

TaiJuWu added 2 commits July 24, 2025 16:09

address indentation comment

b0e292f

fix flaky

391326a

Yunyung reviewed Jul 24, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

Yunyung reviewed Jul 24, 2025

View reviewed changes

address Yunyung comment

e1cfa54

Yunyung reviewed Jul 24, 2025

View reviewed changes

TaiJuWu added 4 commits July 24, 2025 22:16

improve performance

e13bc80

wait shutdown for testResetOffsetsWithPartitionNoneLeader

05b504b

Merge branch 'trunk' into fix_leaderNotFind

ca27cea

Revert "wait shutdown for testResetOffsetsWithPartitionNoneLeader"

7cd0703

This reverts commit 05b504b.

Yunyung reviewed Jul 24, 2025

View reviewed changes

remove wait

cbe8138

chia7712 reviewed Jul 24, 2025

View reviewed changes

address chia comments

3406b22

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

TaiJuWu added 2 commits July 25, 2025 16:41

set min_in_sync

b843e4e

Merge branch 'trunk' into fix_leaderNotFind

ae74eb4

Yunyung reviewed Jul 25, 2025

View reviewed changes

AndrewJSchofield mentioned this pull request Jul 27, 2025

KAFKA-16717 [3/N]: Add AdminClient.alterShareGroupOffsets #19820

Merged

Merge branch 'trunk' into fix_leaderNotFind

5d1c22d

github-actions Bot added the tests Test fixes (including flaky tests) label Jul 30, 2025

TaiJuWu changed the title ~~MINOR: kafka-consumer-groups.sh should not fail when partition offline~~ MINOR: add test for kafka-consumer-groups.sh should not fail when partition offline Jul 30, 2025

address chia comments

1a9e642

fix build

cda748f

JimmyWang6 added a commit to JimmyWang6/jimmy-KAFKA-14048 that referenced this pull request Jul 31, 2025

apply fix apache#20235 for alter offset of stream group

de34d40

JimmyWang6 added a commit to JimmyWang6/jimmy-KAFKA-14048 that referenced this pull request Jul 31, 2025

apply fix apache#20235 for alter offset of stream group

1682455

JimmyWang6 added a commit to JimmyWang6/jimmy-KAFKA-14048 that referenced this pull request Jul 31, 2025

apply fix apache#20235 for alter offset of stream group

a35dd1c

JimmyWang6 mentioned this pull request Jul 31, 2025

MINOR: kafka-stream-groups.sh should fail quickly if the partition leader is unavailable #20271

Merged

AndrewJSchofield approved these changes Jul 31, 2025

View reviewed changes

AndrewJSchofield merged commit dfaf9f9 into apache:trunk Jul 31, 2025
23 checks passed

TaiJuWu deleted the fix_leaderNotFind branch July 31, 2025 15:17

		@ClusterConfigProperty(key = OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "2"),
		@ClusterConfigProperty(key = TopicConfig.MIN_IN_SYNC_REPLICAS_CONFIG, value = "1")

Uh oh!

Conversation

TaiJuWu commented Jul 24, 2025 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TaiJuWu Jul 24, 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!

Yunyung Jul 24, 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!

TaiJuWu Jul 24, 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!

TaiJuWu Jul 25, 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!

This comment was marked as duplicate.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

chia7712 commented Jul 30, 2025

Uh oh!

TaiJuWu commented Jul 30, 2025

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TaiJuWu commented Jul 24, 2025 •

edited by github-actions Bot

Loading

TaiJuWu Jul 24, 2025 •

edited

Loading

Yunyung Jul 24, 2025 •

edited

Loading

TaiJuWu Jul 24, 2025 •

edited

Loading

TaiJuWu Jul 25, 2025 •

edited

Loading