KAFKA-19500: `kafka-consumer-groups.sh` should fail quickly if the partition leader is unavailable by xijiu · Pull Request #20168 · apache/kafka

xijiu · 2025-07-15T06:36:03Z

Add check leader missing logic in method
ConsumerGroupCommand.ConsumerGroupService#prepareOffsetsToReset in
order to fail quickly
Add some tests

Reviewers: TaiJuWu tjwu1217@gmail.com, Lan Ding isDing_L@163.com,
Ken Huang s7133700@gmail.com, Andrew Schofield
aschofield@confluent.io

xijiu · 2025-07-15T06:36:34Z

The all methods which depend on Admin#listOffsets are as follows:

offsetsUtils.resetToOffset
offsetsUtils.resetToEarliest
offsetsUtils.resetToLatest
offsetsUtils.resetByShiftBy
offsetsUtils.resetToDateTime
offsetsUtils.resetByDuration
offsetsUtils.resetFromFile
offsetsUtils.resetToCurrent

And they are all called in method ConsumerGroupCommand.ConsumerGroupService#prepareOffsetsToReset, so we should add the check logic in this method.

AndrewJSchofield

Thanks for the PR. Just one minor comment.

AndrewJSchofield · 2025-07-15T12:45:34Z

+        private void checkAllTopicPartitionsHaveLeader(Collection<TopicPartition> partitionsToReset) {
+            List<TopicPartition> partitionsWithoutLeader = filterNoneLeaderPartitions(partitionsToReset);
+            if (!partitionsWithoutLeader.isEmpty()) {
+                // append the TopicPartition list string


nit: You could do this string concatenation much more neatly with something like

String partitionStr = partitionsWithoutLeader.stream().map(TopicPartition::toString).collect(Collectors.joining(","));

@AndrewJSchofield Thanks very much for CR, it's indeed much neater. I have fixed it, PTAL

m1a2st

Thank @xijiu for this patch, a little comment

m1a2st · 2025-07-15T14:02:53Z

        assertThrows(OptionException.class, () -> getConsumerGroupService(cgcArgs));
    }

+    @Timeout(60)


Nit: No need to add @Timeout here, @ClusterTest already includes a timeout setting.

@m1a2st Thanks very much, the @Timeout is redundant, and I have remove it, PTAL

TaiJuWu

LGTM.

AndrewJSchofield

I think this could do with a little more work. If I create a topic T1 with 1 partition, the following command bin/kafka-consumer-groups.sh --bootstrap-server localhost:9092 --reset-offsets --group CG1 --to-earliest --topic T1:1 --execute still hangs for a long time, even though it has called Admin.describeTopics.

I think it can be improved for the case of a non-existent topic of an existing topic. Seems like it's worth fixing this at the same time.

xijiu · 2025-07-17T07:03:13Z

I think this could do with a little more work. If I create a topic T1 with 1 partition, the following command bin/kafka-consumer-groups.sh --bootstrap-server localhost:9092 --reset-offsets --group CG1 --to-earliest --topic T1:1 --execute still hangs for a long time, even though it has called Admin.describeTopics.

I think it can be improved for the case of a non-existent topic of an existing topic. Seems like it's worth fixing this at the same time.

Good find! It's indeed hangs when a non-existent TopicPartition of an existing topic.

I have added some logic to determine whether the target TopicPartitions actually exists, PTAL

AndrewJSchofield

Thanks for the updates. Just a few minor comments now.

AndrewJSchofield · 2025-07-18T08:52:32Z

+            List<TopicPartition> partitionsNotExistList = filterNotExistPartitions(partitionsToReset);
+            if (!partitionsNotExistList.isEmpty()) {
+                String partitionStr = partitionsNotExistList.stream().map(TopicPartition::toString).collect(Collectors.joining(","));
+                throw new UnknownTopicOrPartitionException("The partitions \"" + partitionStr + "\" does not exist");


nit: Grammar "do not exist" I think.

AndrewJSchofield · 2025-07-18T08:52:40Z

            }
        }

+        private List<TopicPartition> filterNotExistPartitions(Collection<TopicPartition> topicPartitions) {


nit: filterNonExistentPartitions is probably a bit better.

AndrewJSchofield · 2025-07-18T08:55:10Z

+                        .toList();
+
+                return topicPartitions.stream().filter(element -> !existPartitions.contains(element)).toList();
+            } catch (Exception e) {


For consistency, we tend to catch (InterruptedException | ExecutionException e) in this source file.

AndrewJSchofield · 2025-07-18T08:56:17Z

Also please merge latest changes from trunk. Hopefully you'll get a clean test run then.

xijiu · 2025-07-18T17:17:15Z

@AndrewJSchofield Thanks for the code review, and I have fixed them and merged the latest changes from trunk, PTAL

DL1231

LGTM.

AndrewJSchofield

Thanks for the PR. Looks good to me.

chia7712 · 2025-07-22T09:03:51Z

+            }
+
+            // check the partitions have leader
+            List<TopicPartition> partitionsWithoutLeader = filterNoneLeaderPartitions(partitionsToReset);


if there is a topic having three partitions, and the t-2 partition is offline, then if partitionsToReset is t-0,t-1, filterNoneLeaderPartitions will return t-2, causing the tool to fail. Is it expected?

I think it's fair enough.

chia7712@chia7712-ubuntu:~/project/kafka$ ./bin/kafka-consumer-groups.sh \ --bootstrap-server 172.20.10.2:20001 \ --reset-offsets \ --to-earliest \ --execute \ --group perf-consumer-19460 \ --topic chia:1 Error: Executing consumer group command failed due to The partitions "chia-2" have no leader org.apache.kafka.common.errors.LeaderNotAvailableException: The partitions "chia-2" have no leader

It is indeed a bug that unrelated topic partitions could fail the tool.

filterNoneLeaderPartitions needs to get fixed since it could return unrelated topic partitions.

Yes, that's a very good point. Thanks for finding it.

file #20235 to fix it.

Yunyung · 2025-07-24T14:05:54Z

+            produceConsumeAndShutdown(cluster, topic, group, 2, GroupProtocol.CLASSIC);
+            assertDoesNotThrow(() -> resetOffsets(service));
+            // shutdown a broker to make some partitions missing leader
+            cluster.shutdownBroker(0);


Should we wait to ensure it has shut down correctly?

The shutdown already has a 5-minute timeout by default.

…artition offline (#20235) 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>

…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>

…rtition leader is unavailable (apache#20168) 1. Add check leader missing logic in method `ConsumerGroupCommand.ConsumerGroupService#prepareOffsetsToReset` in order to fail quickly 2. Add some tests Reviewers: TaiJuWu <tjwu1217@gmail.com>, Lan Ding <isDing_L@163.com>, Ken Huang <s7133700@gmail.com>, Andrew Schofield <aschofield@confluent.io>

…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>

finish coding

157d929

github-actions Bot added the triage PRs from the community label Jul 15, 2025

github-actions Bot added tools small Small PRs labels Jul 15, 2025

xijiu added ci-approved tools small Small PRs and removed tools small Small PRs labels Jul 15, 2025

fix failed test

5084928

AndrewJSchofield requested changes Jul 15, 2025

View reviewed changes

m1a2st reviewed Jul 15, 2025

View reviewed changes

optimize

edeaa8a

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

TaiJuWu approved these changes Jul 16, 2025

View reviewed changes

m1a2st approved these changes Jul 16, 2025

View reviewed changes

AndrewJSchofield requested changes Jul 16, 2025

View reviewed changes

fix

3efa3bb

AndrewJSchofield requested changes Jul 18, 2025

View reviewed changes

xijiu added 2 commits July 19, 2025 00:26

Merge branch 'trunk' of github.com:xijiu/kafka into 19500

bb09668

fix

dc58674

DL1231 approved these changes Jul 20, 2025

View reviewed changes

AndrewJSchofield approved these changes Jul 21, 2025

View reviewed changes

AndrewJSchofield merged commit f188a31 into apache:trunk Jul 21, 2025
24 checks passed

chia7712 reviewed Jul 22, 2025

View reviewed changes

chia7712 mentioned this pull request Jul 22, 2025

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

Merged

TaiJuWu mentioned this pull request Jul 24, 2025

MINOR: add test for kafka-consumer-groups.sh should not fail when partition offline #20235

Merged

Yunyung reviewed Jul 24, 2025

View reviewed changes

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

apply fix apache#20168 for alter offset of stream group

b3f1ad7

JimmyWang6 mentioned this pull request Jul 31, 2025

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

Merged

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

apply fix apache#20168 for altering offset for stream group

69de610

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

apply fix apache#20168 for altering offset for stream group

f9a87f1

Uh oh!

Conversation

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

Uh oh!

Uh oh!

xijiu commented Jul 15, 2025

Uh oh!

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

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!

TaiJuWu left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

xijiu commented Jul 17, 2025

Uh oh!

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

AndrewJSchofield commented Jul 18, 2025

Uh oh!

xijiu commented Jul 18, 2025

Uh oh!

DL1231 left a comment

Choose a reason for hiding this comment

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!

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!

7 participants

xijiu commented Jul 15, 2025 •

edited by github-actions Bot

Loading