-
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 4 commits
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 |
|---|---|---|
|
|
@@ -41,6 +41,8 @@ | |
| import org.apache.kafka.common.Node; | ||
| 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; | ||
|
|
@@ -1000,6 +1002,9 @@ private Map<TopicPartition, OffsetAndMetadata> getCommittedOffsets(String groupI | |
| } | ||
|
|
||
| private Map<TopicPartition, OffsetAndMetadata> prepareOffsetsToReset(String groupId, Collection<TopicPartition> partitionsToReset) { | ||
| // ensure all partitions are valid, otherwise throw a runtime exception | ||
| checkAllTopicPartitionsValid(partitionsToReset); | ||
|
|
||
| if (opts.options.has(opts.resetToOffsetOpt)) { | ||
| return offsetsUtils.resetToOffset(partitionsToReset); | ||
| } else if (opts.options.has(opts.resetToEarliestOpt)) { | ||
|
|
@@ -1024,6 +1029,38 @@ private Map<TopicPartition, OffsetAndMetadata> prepareOffsetsToReset(String grou | |
| return null; | ||
| } | ||
|
|
||
| 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 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,8 @@ | |
| import org.apache.kafka.clients.producer.ProducerRecord; | ||
| import org.apache.kafka.common.GroupState; | ||
| import org.apache.kafka.common.TopicPartition; | ||
| import org.apache.kafka.common.errors.LeaderNotAvailableException; | ||
| import org.apache.kafka.common.errors.UnknownTopicOrPartitionException; | ||
| import org.apache.kafka.common.serialization.ByteArraySerializer; | ||
| import org.apache.kafka.common.serialization.StringDeserializer; | ||
| import org.apache.kafka.common.test.ClusterInstance; | ||
|
|
@@ -81,6 +83,7 @@ | |
| import static org.apache.kafka.coordinator.group.GroupCoordinatorConfig.OFFSETS_TOPIC_PARTITIONS_CONFIG; | ||
| import static org.apache.kafka.coordinator.group.GroupCoordinatorConfig.OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG; | ||
| import static org.apache.kafka.test.TestUtils.DEFAULT_MAX_WAIT_MS; | ||
| import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
@@ -659,6 +662,41 @@ public void testResetWithUnrecognizedNewConsumerOption(ClusterInstance cluster) | |
| assertThrows(OptionException.class, () -> getConsumerGroupService(cgcArgs)); | ||
| } | ||
|
|
||
| @ClusterTest(brokers = 3, serverProperties = {@ClusterConfigProperty(key = OFFSETS_TOPIC_REPLICATION_FACTOR_CONFIG, value = "2")}) | ||
| public void testResetOffsetsWithPartitionNoneLeader(ClusterInstance cluster) throws Exception { | ||
| String group = generateRandomGroupId(); | ||
| String topic = generateRandomTopic(); | ||
| String[] args = buildArgsForGroup(cluster, group, "--topic", topic + ":0,1,2", | ||
| "--to-earliest", "--execute"); | ||
|
|
||
| try (Admin admin = cluster.admin(); | ||
| ConsumerGroupCommand.ConsumerGroupService service = getConsumerGroupService(args)) { | ||
|
|
||
| admin.createTopics(singleton(new NewTopic(topic, 3, (short) 1))).all().get(); | ||
| produceConsumeAndShutdown(cluster, topic, group, 2, GroupProtocol.CLASSIC); | ||
| assertDoesNotThrow(() -> resetOffsets(service)); | ||
| // shutdown a broker to make some partitions missing leader | ||
| cluster.shutdownBroker(0); | ||
|
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. Should we wait to ensure it has shut down correctly?
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. The shutdown already has a 5-minute timeout by default. |
||
| assertThrows(LeaderNotAvailableException.class, () -> resetOffsets(service)); | ||
| } | ||
| } | ||
|
|
||
| @ClusterTest | ||
| public void testResetOffsetsWithPartitionNotExist(ClusterInstance cluster) throws Exception { | ||
| String group = generateRandomGroupId(); | ||
| String topic = generateRandomTopic(); | ||
| String[] args = buildArgsForGroup(cluster, group, "--topic", topic + ":2,3", | ||
| "--to-earliest", "--execute"); | ||
|
|
||
| try (Admin admin = cluster.admin(); | ||
| ConsumerGroupCommand.ConsumerGroupService service = getConsumerGroupService(args)) { | ||
|
|
||
| admin.createTopics(singleton(new NewTopic(topic, 1, (short) 1))).all().get(); | ||
| produceConsumeAndShutdown(cluster, topic, group, 2, GroupProtocol.CLASSIC); | ||
| assertThrows(UnknownTopicOrPartitionException.class, () -> resetOffsets(service)); | ||
| } | ||
| } | ||
|
|
||
| private String generateRandomTopic() { | ||
| return TOPIC_PREFIX + TestUtils.randomString(10); | ||
| } | ||
|
|
||
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.