-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-19500: kafka-consumer-groups.sh should fail quickly if the partition leader is unavailable
#20168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-19500: kafka-consumer-groups.sh should fail quickly if the partition leader is unavailable
#20168
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ | |
| import org.apache.kafka.common.TopicPartition; | ||
| import org.apache.kafka.common.errors.GroupIdNotFoundException; | ||
| import org.apache.kafka.common.errors.LeaderNotAvailableException; | ||
| import org.apache.kafka.common.errors.UnknownTopicOrPartitionException; | ||
| import org.apache.kafka.common.protocol.Errors; | ||
| import org.apache.kafka.common.utils.Utils; | ||
| import org.apache.kafka.server.util.CommandLineUtils; | ||
|
|
@@ -1001,8 +1002,8 @@ private Map<TopicPartition, OffsetAndMetadata> getCommittedOffsets(String groupI | |
| } | ||
|
|
||
| private Map<TopicPartition, OffsetAndMetadata> prepareOffsetsToReset(String groupId, Collection<TopicPartition> partitionsToReset) { | ||
| // ensure all partitions have leader, otherwise throw a runtime exception | ||
| checkAllTopicPartitionsHaveLeader(partitionsToReset); | ||
| // ensure all partitions are valid, otherwise throw a runtime exception | ||
| checkAllTopicPartitionsValid(partitionsToReset); | ||
|
|
||
| if (opts.options.has(opts.resetToOffsetOpt)) { | ||
| return offsetsUtils.resetToOffset(partitionsToReset); | ||
|
|
@@ -1028,15 +1029,38 @@ private Map<TopicPartition, OffsetAndMetadata> prepareOffsetsToReset(String grou | |
| return null; | ||
| } | ||
|
|
||
| private void checkAllTopicPartitionsHaveLeader(Collection<TopicPartition> partitionsToReset) { | ||
| private void checkAllTopicPartitionsValid(Collection<TopicPartition> partitionsToReset) { | ||
| // check the partitions exist | ||
| 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"); | ||
| } | ||
|
|
||
| // check the partitions have leader | ||
| List<TopicPartition> partitionsWithoutLeader = filterNoneLeaderPartitions(partitionsToReset); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if there is a topic having three partitions, and the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fair enough.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is indeed a bug that unrelated topic partitions could fail the tool.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's a very good point. Thanks for finding it.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. file #20235 to fix it. |
||
| if (!partitionsWithoutLeader.isEmpty()) { | ||
| String partitionStr = partitionsWithoutLeader.stream().map(TopicPartition::toString).collect(Collectors.joining(",")); | ||
| // throw exception | ||
| throw new LeaderNotAvailableException("The partitions \"" + partitionStr + "\" have no leader"); | ||
| } | ||
| } | ||
|
|
||
| private List<TopicPartition> filterNotExistPartitions(Collection<TopicPartition> topicPartitions) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| // collect all topics | ||
| Set<String> topics = topicPartitions.stream().map(TopicPartition::topic).collect(Collectors.toSet()); | ||
| try { | ||
| List<TopicPartition> existPartitions = adminClient.describeTopics(topics).allTopicNames().get().entrySet() | ||
| .stream() | ||
| .flatMap(entry -> entry.getValue().partitions().stream() | ||
| .map(partitionInfo -> new TopicPartition(entry.getKey(), partitionInfo.partition()))) | ||
| .toList(); | ||
|
|
||
| return topicPartitions.stream().filter(element -> !existPartitions.contains(element)).toList(); | ||
| } catch (Exception e) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency, we tend to |
||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| String exportOffsetsToCsv(Map<String, Map<TopicPartition, OffsetAndMetadata>> assignments) { | ||
| boolean isSingleGroupQuery = opts.options.valuesOf(opts.groupOpt).size() == 1; | ||
| ObjectWriter csvWriter = isSingleGroupQuery | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Grammar "do not exist" I think.