Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
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.protocol.Errors;
import org.apache.kafka.common.utils.Utils;
import org.apache.kafka.server.util.CommandLineUtils;
Expand Down Expand Up @@ -1000,6 +1001,9 @@ 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);

if (opts.options.has(opts.resetToOffsetOpt)) {
return offsetsUtils.resetToOffset(partitionsToReset);
} else if (opts.options.has(opts.resetToEarliestOpt)) {
Expand All @@ -1024,6 +1028,15 @@ private Map<TopicPartition, OffsetAndMetadata> prepareOffsetsToReset(String grou
return null;
}

private void checkAllTopicPartitionsHaveLeader(Collection<TopicPartition> partitionsToReset) {
List<TopicPartition> partitionsWithoutLeader = filterNoneLeaderPartitions(partitionsToReset);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fair enough.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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");
}
}

String exportOffsetsToCsv(Map<String, Map<TopicPartition, OffsetAndMetadata>> assignments) {
boolean isSingleGroupQuery = opts.options.valuesOf(opts.groupOpt).size() == 1;
ObjectWriter csvWriter = isSingleGroupQuery
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anySet;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -234,6 +235,8 @@ public void testAdminRequestsForResetOffsets() {
.thenReturn(describeGroupsResult(GroupState.DEAD));
when(admin.describeTopics(ArgumentMatchers.eq(topicsWithoutPartitionsSpecified), any()))
.thenReturn(describeTopicsResult(topicsWithoutPartitionsSpecified));
when(admin.describeTopics(anySet()))
.thenReturn(describeTopicsResult(topicsWithoutPartitionsSpecified));
when(admin.listOffsets(offsetsArgMatcher(), any()))
.thenReturn(listOffsetsResult());

Expand Down Expand Up @@ -317,7 +320,7 @@ private DescribeTopicsResult describeTopicsResult(Collection<String> topics) {

topics.forEach(topic -> {
List<TopicPartitionInfo> partitions = IntStream.range(0, NUM_PARTITIONS)
.mapToObj(i -> new TopicPartitionInfo(i, null, Collections.emptyList(), Collections.emptyList()))
.mapToObj(i -> new TopicPartitionInfo(i, Node.noNode(), Collections.emptyList(), Collections.emptyList()))
.collect(Collectors.toList());
topicDescriptions.put(topic, new TopicDescription(topic, false, partitions));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
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.serialization.ByteArraySerializer;
import org.apache.kafka.common.serialization.StringDeserializer;
import org.apache.kafka.common.test.ClusterInstance;
Expand Down Expand Up @@ -81,6 +82,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;
Expand Down Expand Up @@ -659,6 +661,25 @@ 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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we wait to ensure it has shut down correctly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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));
}
}

private String generateRandomTopic() {
return TOPIC_PREFIX + TestUtils.randomString(10);
}
Expand Down
Loading