From 277e6a5c59d8e1e52324720fd262badd247ad3e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Wed, 12 Aug 2020 11:57:18 -0700 Subject: [PATCH 01/10] KAFKA-10427: Basic fetch snapshot implementation --- .../errors/SnapshotNotFoundException.java | 31 +++ .../apache/kafka/common/protocol/ApiKeys.java | 6 +- .../apache/kafka/common/protocol/Errors.java | 4 +- .../common/requests/FetchSnapshotRequest.java | 84 ++++++++ .../requests/FetchSnapshotResponse.java | 113 ++++++++++ .../common/message/FetchSnapshotRequest.json | 46 ++++ .../common/message/FetchSnapshotResponse.json | 59 ++++++ .../main/scala/kafka/server/KafkaApis.scala | 1 + .../kafka/tools/TestRaftRequestHandler.scala | 3 +- .../apache/kafka/raft/KafkaRaftClient.java | 145 ++++++++++++- .../raft/KafkaRaftClientSnapshotTest.java | 197 ++++++++++++++++++ .../java/org/apache/kafka/raft/MockLog.java | 1 + .../kafka/raft/RaftClientTestContext.java | 19 +- 13 files changed, 703 insertions(+), 6 deletions(-) create mode 100644 clients/src/main/java/org/apache/kafka/common/errors/SnapshotNotFoundException.java create mode 100644 clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java create mode 100644 clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java create mode 100644 clients/src/main/resources/common/message/FetchSnapshotRequest.json create mode 100644 clients/src/main/resources/common/message/FetchSnapshotResponse.json create mode 100644 raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java diff --git a/clients/src/main/java/org/apache/kafka/common/errors/SnapshotNotFoundException.java b/clients/src/main/java/org/apache/kafka/common/errors/SnapshotNotFoundException.java new file mode 100644 index 0000000000000..5b3e7ed1605ec --- /dev/null +++ b/clients/src/main/java/org/apache/kafka/common/errors/SnapshotNotFoundException.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.common.errors; + +public class SnapshotNotFoundException extends ApiException { + + private static final long serialVersionUID = 1; + + public SnapshotNotFoundException(String s) { + super(s); + } + + public SnapshotNotFoundException(String message, Throwable cause) { + super(message, cause); + } + +} diff --git a/clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java b/clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java index c48e6e100364a..6d74a1a838e32 100644 --- a/clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java +++ b/clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java @@ -83,6 +83,8 @@ import org.apache.kafka.common.message.ExpireDelegationTokenResponseData; import org.apache.kafka.common.message.FetchRequestData; import org.apache.kafka.common.message.FetchResponseData; +import org.apache.kafka.common.message.FetchSnapshotRequestData; +import org.apache.kafka.common.message.FetchSnapshotResponseData; import org.apache.kafka.common.message.FindCoordinatorRequestData; import org.apache.kafka.common.message.FindCoordinatorResponseData; import org.apache.kafka.common.message.HeartbeatRequestData; @@ -254,7 +256,9 @@ public Struct parseResponse(short version, ByteBuffer buffer) { ALTER_ISR(56, "AlterIsr", AlterIsrRequestData.SCHEMAS, AlterIsrResponseData.SCHEMAS), UPDATE_FEATURES(57, "UpdateFeatures", UpdateFeaturesRequestData.SCHEMAS, UpdateFeaturesResponseData.SCHEMAS, true), - ENVELOPE(58, "Envelope", true, false, EnvelopeRequestData.SCHEMAS, EnvelopeResponseData.SCHEMAS); + ENVELOPE(58, "Envelope", true, false, EnvelopeRequestData.SCHEMAS, EnvelopeResponseData.SCHEMAS), + FETCH_SNAPSHOT(59, "FetchSnapshot", false, false, + FetchSnapshotRequestData.SCHEMAS, FetchSnapshotResponseData.SCHEMAS); private static final ApiKeys[] ID_TO_TYPE; private static final int MIN_API_KEY = 0; diff --git a/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java b/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java index 5332369f0f9be..77498c4f2ee41 100644 --- a/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java +++ b/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java @@ -95,6 +95,7 @@ import org.apache.kafka.common.errors.RetriableException; import org.apache.kafka.common.errors.SaslAuthenticationException; import org.apache.kafka.common.errors.SecurityDisabledException; +import org.apache.kafka.common.errors.SnapshotNotFoundException; import org.apache.kafka.common.errors.StaleBrokerEpochException; import org.apache.kafka.common.errors.ThrottlingQuotaExceededException; import org.apache.kafka.common.errors.TimeoutException; @@ -341,7 +342,8 @@ public enum Errors { INVALID_UPDATE_VERSION(95, "The given update version was invalid.", InvalidUpdateVersionException::new), FEATURE_UPDATE_FAILED(96, "Unable to update finalized features due to an unexpected server error.", FeatureUpdateFailedException::new), PRINCIPAL_DESERIALIZATION_FAILURE(97, "Request principal deserialization failed during forwarding. " + - "This indicates an internal error on the broker cluster security setup.", PrincipalDeserializationException::new); + "This indicates an internal error on the broker cluster security setup.", PrincipalDeserializationException::new), + SNAPSHOT_NOT_FOUND(98, "Requested snapshot was not found", SnapshotNotFoundException::new); private static final Logger log = LoggerFactory.getLogger(Errors.class); diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java new file mode 100644 index 0000000000000..2c4f195320e76 --- /dev/null +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.common.requests; + +import java.util.Collections; +import java.util.Optional; +import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.message.FetchSnapshotRequestData; +import org.apache.kafka.common.message.FetchSnapshotResponseData; +import org.apache.kafka.common.protocol.ApiKeys; +import org.apache.kafka.common.protocol.Errors; +import org.apache.kafka.common.protocol.types.Struct; + +final public class FetchSnapshotRequest extends AbstractRequest { + public final FetchSnapshotRequestData data; + + public FetchSnapshotRequest(FetchSnapshotRequestData data) { + super(ApiKeys.FETCH_SNAPSHOT, (short) (FetchSnapshotRequestData.SCHEMAS.length - 1)); + this.data = data; + } + + @Override + protected Struct toStruct() { + return data.toStruct(version()); + } + + @Override + public FetchSnapshotResponse getErrorResponse(int throttleTimeMs, Throwable e) { + // TODO: we need to handle throttleTimeMs + return new FetchSnapshotResponse(new FetchSnapshotResponseData().setErrorCode(Errors.forException(e).code())); + } + + public static FetchSnapshotRequestData singleton( + TopicPartition topicPartition, + FetchSnapshotRequestData.SnapshotId snapshotId, + int maxBytes, + long position + ) { + return new FetchSnapshotRequestData() + .setMaxBytes(maxBytes) + .setTopics( + Collections.singletonList( + new FetchSnapshotRequestData.TopicSnapshot() + .setName(topicPartition.topic()) + .setPartitions( + Collections.singletonList( + new FetchSnapshotRequestData.PartitionSnapshot() + .setIndex(topicPartition.partition()) + .setSnapshotId(snapshotId) + .setPosition(position) + ) + ) + ) + ); + } + + // TODO: write documentation. This function assumes that topic partitions are unique in `data` + public static Optional forTopicPartition( + FetchSnapshotRequestData data, + TopicPartition topicPartition + ) { + return data + .topics() + .stream() + .filter(topic -> topic.name().equals(topicPartition.topic())) + .flatMap(topic -> topic.partitions().stream()) + .filter(parition -> parition.index() == topicPartition.partition()) + .findAny(); + } +} diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java new file mode 100644 index 0000000000000..878e15b4fb656 --- /dev/null +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.common.requests; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.function.UnaryOperator; +import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.message.FetchSnapshotResponseData; +import org.apache.kafka.common.protocol.Errors; +import org.apache.kafka.common.protocol.types.Struct; + +final public class FetchSnapshotResponse extends AbstractResponse { + public final FetchSnapshotResponseData data; + + public FetchSnapshotResponse(FetchSnapshotResponseData data) { + this.data = data; + } + + @Override + protected Struct toStruct(short version) { + return data.toStruct(version); + } + + @Override + public Map errorCounts() { + Map errors = new HashMap<>(); + + Errors topLevelError = Errors.forCode(data.errorCode()); + if (topLevelError != Errors.NONE) { + errors.put(topLevelError, 1); + } + + for (FetchSnapshotResponseData.TopicSnapshot topicResponse : data.topics()) { + for (FetchSnapshotResponseData.PartitionSnapshot partitionResponse : topicResponse.partitions()) { + errors.compute(Errors.forCode(partitionResponse.errorCode()), + (error, count) -> count == null ? 1 : count + 1); + } + } + + return errors; + } + + public static FetchSnapshotResponseData withTopError(Errors error) { + return new FetchSnapshotResponseData().setErrorCode(error.code()); + } + + public static FetchSnapshotResponseData singletonWithError(TopicPartition topicPartition, Errors error) { + return new FetchSnapshotResponseData() + .setTopics( + Collections.singletonList( + new FetchSnapshotResponseData.TopicSnapshot() + .setName(topicPartition.topic()) + .setPartitions( + Collections.singletonList( + new FetchSnapshotResponseData.PartitionSnapshot() + .setIndex(topicPartition.partition()) + .setErrorCode(error.code()) + ) + ) + ) + ); + } + + public static FetchSnapshotResponseData singletonWithData( + TopicPartition topicPartition, + UnaryOperator operator + ) { + FetchSnapshotResponseData.PartitionSnapshot partitionSnapshot = operator.apply( + new FetchSnapshotResponseData.PartitionSnapshot().setIndex(topicPartition.partition()) + ); + + return new FetchSnapshotResponseData() + .setTopics( + Collections.singletonList( + new FetchSnapshotResponseData.TopicSnapshot() + .setName(topicPartition.topic()) + .setPartitions(Collections.singletonList(partitionSnapshot)) + ) + ); + } + + // TODO: write documentation. This function assumes that topic partitions are unique in `data` + public static Optional forTopicPartition( + FetchSnapshotResponseData data, + TopicPartition topicPartition + ) { + return data + .topics() + .stream() + .filter(topic -> topic.name().equals(topicPartition.topic())) + .flatMap(topic -> topic.partitions().stream()) + .filter(parition -> parition.index() == topicPartition.partition()) + .findAny(); + } + +} diff --git a/clients/src/main/resources/common/message/FetchSnapshotRequest.json b/clients/src/main/resources/common/message/FetchSnapshotRequest.json new file mode 100644 index 0000000000000..1467ed5114c94 --- /dev/null +++ b/clients/src/main/resources/common/message/FetchSnapshotRequest.json @@ -0,0 +1,46 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +{ + "apiKey": 59, + "type": "request", + "name": "FetchSnapshotRequest", + "validVersions": "0", + "flexibleVersions": "0+", + "fields": [ + { "name": "ReplicaId", "type": "int32", "versions": "0+", + "about": "The broker ID of the follower." }, + { "name": "MaxBytes", "type": "int32", "versions": "0+", + "about": "The maximum bytes to fetch from all of the snapshots." }, + { "name": "Topics", "type": "[]TopicSnapshot", "versions": "0+", + "about": "The topics to fetch.", "fields": [ + { "name": "Name", "type": "string", "versions": "0+", "entityType": "topicName", + "about": "The name of the topic to fetch." }, + { "name": "Partitions", "type": "[]PartitionSnapshot", "versions": "0+", + "about": "The partitions to fetch.", "fields": [ + { "name": "Index", "type": "int32", "versions": "0+", + "about": "The partition index." }, + { "name": "SnapshotId", "type": "SnapshotId", "versions": "0+", + "about": "The snapshot endOffset and epoch to fetch", + "fields": [ + { "name": "EndOffset", "type": "int64", "versions": "0+" }, + { "name": "Epoch", "type": "int32", "versions": "0+" } + ]}, + { "name": "Position", "type": "int64", "versions": "0+", + "about": "The byte position within the snapshot to start fetching from." } + ]} + ]} + ] +} diff --git a/clients/src/main/resources/common/message/FetchSnapshotResponse.json b/clients/src/main/resources/common/message/FetchSnapshotResponse.json new file mode 100644 index 0000000000000..711b5361c9546 --- /dev/null +++ b/clients/src/main/resources/common/message/FetchSnapshotResponse.json @@ -0,0 +1,59 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +{ + "apiKey": 59, + "type": "response", + "name": "FetchSnapshotResponse", + "validVersions": "0", + "flexibleVersions": "0+", + "fields": [ + { "name": "ThrottleTimeMs", "type": "int32", "versions": "0+", "ignorable": true, + "about": "The duration in milliseconds for which the request was throttled due to a quota violation, or zero if the request did not violate any quota." }, + { "name": "ErrorCode", "type": "int16", "versions": "0+", "ignorable": false, + "about": "The top level response error code." }, + { "name": "Topics", "type": "[]TopicSnapshot", "versions": "0+", + "about": "The topics to fetch.", "fields": [ + { "name": "Name", "type": "string", "versions": "0+", "entityType": "topicName", + "about": "The name of the topic to fetch." }, + { "name": "Partitions", "type": "[]PartitionSnapshot", "versions": "0+", + "about": "The partitions to fetch.", "fields": [ + { "name": "Index", "type": "int32", "versions": "0+", + "about": "The partition index." }, + { "name": "ErrorCode", "type": "int16", "versions": "0+", + "about": "The error code, or 0 if there was no fetch error." }, + { "name": "SnapshotId", "type": "SnapshotId", "versions": "0+", + "about": "The snapshot endOffset and epoch fetched", + "fields": [ + { "name": "EndOffset", "type": "int64", "versions": "0+" }, + { "name": "Epoch", "type": "int32", "versions": "0+" } + ]}, + { "name": "CurrentLeader", "type": "LeaderIdAndEpoch", + "versions": "0+", "taggedVersions": "0+", "tag": 0, "fields": [ + { "name": "LeaderId", "type": "int32", "versions": "0+", + "about": "The ID of the current leader or -1 if the leader is unknown."}, + { "name": "LeaderEpoch", "type": "int32", "versions": "0+", + "about": "The latest known leader epoch"} + ]}, + { "name": "Size", "type": "int64", "versions": "0+", + "about": "The total size of the snapshot." }, + { "name": "Position", "type": "int64", "versions": "0+", + "about": "The starting byte position within the snapshot included in the Bytes field." }, + { "name": "Bytes", "type": "bytes", "versions": "0+", "zeroCopy": true, + "about": "Snapshot data." } + ]} + ]} + ] +} diff --git a/core/src/main/scala/kafka/server/KafkaApis.scala b/core/src/main/scala/kafka/server/KafkaApis.scala index 6aa71505acf4f..147730a6d94a3 100644 --- a/core/src/main/scala/kafka/server/KafkaApis.scala +++ b/core/src/main/scala/kafka/server/KafkaApis.scala @@ -256,6 +256,7 @@ class KafkaApis(val requestChannel: RequestChannel, case ApiKeys.BEGIN_QUORUM_EPOCH => closeConnection(request, util.Collections.emptyMap()) case ApiKeys.END_QUORUM_EPOCH => closeConnection(request, util.Collections.emptyMap()) case ApiKeys.DESCRIBE_QUORUM => closeConnection(request, util.Collections.emptyMap()) + case ApiKeys.FETCH_SNAPSHOT => closeConnection(request, util.Collections.emptyMap()) } } catch { case e: FatalExitError => throw e diff --git a/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala b/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala index fefd274927df2..d1f87668a304c 100644 --- a/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala +++ b/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala @@ -45,7 +45,8 @@ class TestRaftRequestHandler( case ApiKeys.VOTE | ApiKeys.BEGIN_QUORUM_EPOCH | ApiKeys.END_QUORUM_EPOCH - | ApiKeys.FETCH => + | ApiKeys.FETCH + | ApiKeys.FETCH_SNAPSHOT => val requestBody = request.body[AbstractRequest] networkChannel.postInboundRequest(requestBody, response => sendResponse(request, Some(response))) diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index 37c1846047af5..6ea9b76c68160 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -17,6 +17,7 @@ package org.apache.kafka.raft; import org.apache.kafka.common.KafkaException; +import org.apache.kafka.common.TopicPartition; import org.apache.kafka.common.errors.ClusterAuthorizationException; import org.apache.kafka.common.errors.NotLeaderOrFollowerException; import org.apache.kafka.common.memory.MemoryPool; @@ -29,6 +30,8 @@ import org.apache.kafka.common.message.EndQuorumEpochResponseData; import org.apache.kafka.common.message.FetchRequestData; import org.apache.kafka.common.message.FetchResponseData; +import org.apache.kafka.common.message.FetchSnapshotRequestData; +import org.apache.kafka.common.message.FetchSnapshotResponseData; import org.apache.kafka.common.message.LeaderChangeMessage.Voter; import org.apache.kafka.common.message.LeaderChangeMessage; import org.apache.kafka.common.message.VoteRequestData; @@ -47,6 +50,8 @@ import org.apache.kafka.common.requests.DescribeQuorumResponse; import org.apache.kafka.common.requests.EndQuorumEpochRequest; import org.apache.kafka.common.requests.EndQuorumEpochResponse; +import org.apache.kafka.common.requests.FetchSnapshotRequest; +import org.apache.kafka.common.requests.FetchSnapshotResponse; import org.apache.kafka.common.requests.VoteRequest; import org.apache.kafka.common.requests.VoteResponse; import org.apache.kafka.common.utils.LogContext; @@ -61,11 +66,13 @@ import org.apache.kafka.raft.internals.MemoryBatchReader; import org.apache.kafka.raft.internals.RecordsBatchReader; import org.apache.kafka.raft.internals.ThresholdPurgatory; +import org.apache.kafka.snapshot.RawSnapshotReader; import org.apache.kafka.snapshot.SnapshotWriter; import org.slf4j.Logger; import java.io.IOException; import java.net.InetSocketAddress; +import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collections; import java.util.Iterator; @@ -983,7 +990,7 @@ private Optional validateFetchOffsetAndEpoch(long fetchOffset, i } } - private OptionalInt optionalLeaderId(int leaderIdOrNil) { + private static OptionalInt optionalLeaderId(int leaderIdOrNil) { if (leaderIdOrNil < 0) return OptionalInt.empty(); return OptionalInt.of(leaderIdOrNil); @@ -1101,6 +1108,125 @@ private DescribeQuorumResponseData handleDescribeQuorumRequest( ); } + private FetchSnapshotResponseData handleFetchSnapshotRequest( + RaftRequest.Inbound requestMetadata + ) throws IOException { + FetchSnapshotRequestData data = (FetchSnapshotRequestData) requestMetadata.data; + + if (data.topics().size() != 1 && data.topics().get(0).partitions().size() != 1) { + return FetchSnapshotResponse.withTopError(Errors.INVALID_REQUEST); + } + + Optional partitionSnapshotOpt = FetchSnapshotRequest + .forTopicPartition(data, log.topicPartition()); + if (!partitionSnapshotOpt.isPresent()) { + // The Raft client assumes that there is only one topic partition. + TopicPartition unknownTopicPartition = new TopicPartition( + data.topics().get(0).name(), + data.topics().get(0).partitions().get(0).index() + ); + + return FetchSnapshotResponse.singletonWithError(unknownTopicPartition, Errors.UNKNOWN_TOPIC_OR_PARTITION); + } + + if (!quorum.isLeader()) { + return FetchSnapshotResponse.singletonWithData( + log.topicPartition(), + partitionSnapshot -> { + partitionSnapshot.currentLeader() + .setLeaderEpoch(quorum.epoch()) + .setLeaderId(quorum.leaderIdOrNil()); + + return partitionSnapshot; + } + ); + } + + FetchSnapshotRequestData.PartitionSnapshot partitionSnapshot = partitionSnapshotOpt.get(); + OffsetAndEpoch snapshotId = new OffsetAndEpoch( + partitionSnapshot.snapshotId().endOffset(), + partitionSnapshot.snapshotId().epoch() + ); + + + Optional snapshotOpt = log.readSnapshot(snapshotId); + if (!snapshotOpt.isPresent()) { + return FetchSnapshotResponse.singletonWithError(log.topicPartition(), Errors.SNAPSHOT_NOT_FOUND); + } + + try (RawSnapshotReader snapshot = snapshotOpt.get()) { + int maxSnapshotSize; + try { + maxSnapshotSize = Math.toIntExact(snapshot.sizeInBytes()); + } catch (ArithmeticException e) { + maxSnapshotSize = Integer.MAX_VALUE; + } + // TODO: Make sure that we also limit based on the fetch max bytes configuration + ByteBuffer buffer = ByteBuffer.allocate(Math.min(data.maxBytes(), maxSnapshotSize)); + snapshot.read(buffer, partitionSnapshot.position()); + buffer.flip(); + + long snapshotSize = snapshot.sizeInBytes(); + + return FetchSnapshotResponse.singletonWithData( + log.topicPartition(), + responsePartitionSnapshot -> { + return responsePartitionSnapshot + .setSize(snapshotSize) + .setPosition(partitionSnapshot.position()) + .setBytes(buffer); + } + ); + } + } + + private boolean handleFetchSnapshotResponse( + RaftResponse.Inbound responseMetadata, + long currentTimeMs + ) throws IOException { + FetchSnapshotResponseData data = (FetchSnapshotResponseData) responseMetadata.data; + Errors topLevelError = Errors.forCode(data.errorCode()); + if (topLevelError != Errors.NONE) { + return handleTopLevelError(topLevelError, responseMetadata); + } + + if (data.topics().size() != 1 && data.topics().get(0).partitions().size() != 1) { + return false; + } + + Optional partitionSnapshotOpt = FetchSnapshotResponse + .forTopicPartition(data, log.topicPartition()); + if (!partitionSnapshotOpt.isPresent()) { + return false; + } + + FetchSnapshotResponseData.PartitionSnapshot partitionSnapshot = partitionSnapshotOpt.get(); + + FetchSnapshotResponseData.LeaderIdAndEpoch currentLeaderIdAndEpoch = partitionSnapshot.currentLeader(); + OptionalInt responseLeaderId = optionalLeaderId(currentLeaderIdAndEpoch.leaderId()); + int responseEpoch = currentLeaderIdAndEpoch.leaderEpoch(); + Errors error = Errors.forCode(partitionSnapshot.errorCode()); + + Optional handled = maybeHandleCommonResponse( + error, responseLeaderId, responseEpoch, currentTimeMs); + if (handled.isPresent()) { + return handled.get(); + } + + /* + // TODO: uncomment this block + OffsetAndEpoch snapshotId = new OffsetAndEpoch( + partitionSnapshot.snapshotId().endOffset(), + partitionSnapshot.snapshotId().epoch() + ); + */ + + + // TODO: handle the rest of the response + + return true; + } + List convertToReplicaStates(Map replicaEndOffsets) { return replicaEndOffsets.entrySet().stream() .map(entry -> new ReplicaState() @@ -1247,6 +1373,10 @@ private void handleResponse(RaftResponse.Inbound response, long currentTimeMs) t handledSuccessfully = handleEndQuorumEpochResponse(response, currentTimeMs); break; + case FETCH_SNAPSHOT: + handledSuccessfully = handleFetchSnapshotResponse(response, currentTimeMs); + break; + default: throw new IllegalArgumentException("Received unexpected response type: " + apiKey); } @@ -1322,6 +1452,10 @@ private void handleRequest(RaftRequest.Inbound request, long currentTimeMs) thro responseFuture = completedFuture(handleDescribeQuorumRequest(request, currentTimeMs)); break; + case FETCH_SNAPSHOT: + responseFuture = completedFuture(handleFetchSnapshotRequest(request)); + break; + default: throw new IllegalArgumentException("Unexpected request type " + apiKey); } @@ -1451,6 +1585,15 @@ private long maybeSendAnyVoterFetch(long currentTimeMs) { } } + private FetchSnapshotRequestData buildFetchSnapshotRequest() { + // TODO: fully implement this: how do we find out what snapshot we are suppose to fetch? + return new FetchSnapshotRequestData(); + } + + private void maybeSendFetchSnapshot(long currentTimeMs, int leaderId) throws IOException { + maybeSendRequest(currentTimeMs, leaderId, this::buildFetchSnapshotRequest); + } + public boolean isRunning() { GracefulShutdown gracefulShutdown = shutdown.get(); return gracefulShutdown == null || !gracefulShutdown.isFinished(); diff --git a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java new file mode 100644 index 0000000000000..0a5266fa5aad0 --- /dev/null +++ b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java @@ -0,0 +1,197 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.raft; + +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.message.FetchSnapshotRequestData; +import org.apache.kafka.common.message.FetchSnapshotResponseData; +import org.apache.kafka.common.protocol.Errors; +import org.apache.kafka.common.requests.FetchSnapshotRequest; +import org.apache.kafka.common.utils.Utils; +import org.apache.kafka.snapshot.SnapshotWriter; +import org.apache.kafka.snapshot.RawSnapshotReader; +import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertEquals; + +final public class KafkaRaftClientSnapshotTest { + @Test + public void testMissingFetchSnapshotRequest() throws Exception { + int localId = 0; + int epoch = 2; + Set voters = Utils.mkSet(localId, localId + 1); + + RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); + + context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, new OffsetAndEpoch(0, 0), Integer.MAX_VALUE, 0)); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + assertEquals(Errors.SNAPSHOT_NOT_FOUND, Errors.forCode(response.errorCode())); + } + + @Test + public void testUnknownFetchSnapshotRequest() throws Exception { + int localId = 0; + Set voters = Utils.mkSet(localId, localId + 1); + int epoch = 2; + TopicPartition topicPartition = new TopicPartition("unknown", 0); + + RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); + + context.deliverRequest(fetchSnapshotRequest(topicPartition, new OffsetAndEpoch(0, 0), Integer.MAX_VALUE, 0)); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(topicPartition).get(); + assertEquals(Errors.UNKNOWN_TOPIC_OR_PARTITION, Errors.forCode(response.errorCode())); + } + + @Test + public void testFetchSnapshotRequestAsLeader() throws Exception { + int localId = 0; + Set voters = Utils.mkSet(localId, localId + 1); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(0, 0); + List records = Arrays.asList("foo", "bar"); + + RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); + + try (SnapshotWriter snapshot = context.client.createSnapshot(snapshotId)) { + snapshot.append(records); + snapshot.freeze(); + } + + try (RawSnapshotReader snapshot = context.log.readSnapshot(snapshotId).get()) { + context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, snapshotId, Integer.MAX_VALUE, 0)); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context + .assertSentFetchSnapshotResponse(context.metadataPartition) + .get(); + + assertEquals(Errors.NONE, Errors.forCode(response.errorCode())); + assertEquals(snapshot.sizeInBytes(), response.size()); + assertEquals(0, response.position()); + assertEquals(snapshot.sizeInBytes(), response.bytes().remaining()); + + ByteBuffer buffer = ByteBuffer.allocate((int) snapshot.sizeInBytes()); + snapshot.read(buffer, 0); + buffer.flip(); + + assertEquals(buffer.slice(), response.bytes()); + } + } + + @Test + public void testPartialFetchSnapshotRequestAsLeader() throws Exception { + int localId = 0; + Set voters = Utils.mkSet(localId, localId + 1); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(0, 0); + List records = Arrays.asList("foo", "bar"); + + RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); + + try (SnapshotWriter snapshot = context.client.createSnapshot(snapshotId)) { + snapshot.append(records); + snapshot.freeze(); + } + + try (RawSnapshotReader snapshot = context.log.readSnapshot(snapshotId).get()) { + // Fetch half of the snapshot + context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, snapshotId, (int) snapshot.sizeInBytes() / 2, 0)); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + + assertEquals(Errors.NONE, Errors.forCode(response.errorCode())); + assertEquals(snapshot.sizeInBytes(), response.size()); + assertEquals(0, response.position()); + assertEquals(snapshot.sizeInBytes() / 2, response.bytes().remaining()); + + ByteBuffer snapshotBuffer = ByteBuffer.allocate((int) snapshot.sizeInBytes()); + snapshot.read(snapshotBuffer, 0); + snapshotBuffer.flip(); + + ByteBuffer responseBuffer = ByteBuffer.allocate((int) snapshot.sizeInBytes()); + responseBuffer.put(response.bytes()); + + ByteBuffer expectedBytes = snapshotBuffer.duplicate(); + expectedBytes.limit((int) snapshot.sizeInBytes() / 2); + + assertEquals(expectedBytes, responseBuffer.duplicate().flip()); + + // Fetch the remainder of the snapshot + context.deliverRequest( + fetchSnapshotRequest(context.metadataPartition, snapshotId, Integer.MAX_VALUE, responseBuffer.position()) + ); + + context.client.poll(); + + response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + assertEquals(Errors.NONE, Errors.forCode(response.errorCode())); + assertEquals(snapshot.sizeInBytes(), response.size()); + assertEquals(responseBuffer.position(), response.position()); + assertEquals(snapshot.sizeInBytes() - (snapshot.sizeInBytes() / 2), response.bytes().remaining()); + + responseBuffer.put(response.bytes()); + assertEquals(snapshotBuffer, responseBuffer.flip()); + } + } + + @Test + public void testFetchSnapshotRequestAsFollower() throws IOException { + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(0, 0); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, snapshotId, Integer.MAX_VALUE, 0)); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + assertEquals(Errors.NONE, Errors.forCode(response.errorCode())); + assertEquals(epoch, response.currentLeader().leaderEpoch()); + assertEquals(leaderId, response.currentLeader().leaderId()); + } + + private static FetchSnapshotRequestData fetchSnapshotRequest( + TopicPartition topicPartition, + OffsetAndEpoch offsetAndEpoch, + int maxBytes, + long position + ) { + FetchSnapshotRequestData.SnapshotId snapshotId = new FetchSnapshotRequestData.SnapshotId() + .setEndOffset(offsetAndEpoch.offset) + .setEpoch(offsetAndEpoch.epoch); + return FetchSnapshotRequest.singleton(topicPartition, snapshotId, maxBytes, position); + } +} diff --git a/raft/src/test/java/org/apache/kafka/raft/MockLog.java b/raft/src/test/java/org/apache/kafka/raft/MockLog.java index 1e8c1b53fcb72..1e5fbb092b07c 100644 --- a/raft/src/test/java/org/apache/kafka/raft/MockLog.java +++ b/raft/src/test/java/org/apache/kafka/raft/MockLog.java @@ -21,6 +21,7 @@ import org.apache.kafka.common.record.CompressionType; import org.apache.kafka.common.record.MemoryRecords; import org.apache.kafka.common.record.MemoryRecordsBuilder; +import org.apache.kafka.common.record.MutableRecordBatch; import org.apache.kafka.common.record.Record; import org.apache.kafka.common.record.RecordBatch; import org.apache.kafka.common.record.Records; diff --git a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java index 0e803e5ebef4b..3d498ae7ca71b 100644 --- a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java +++ b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java @@ -20,14 +20,15 @@ import org.apache.kafka.common.memory.MemoryPool; import org.apache.kafka.common.message.BeginQuorumEpochRequestData; import org.apache.kafka.common.message.BeginQuorumEpochResponseData; -import org.apache.kafka.common.message.DescribeQuorumResponseData; import org.apache.kafka.common.message.DescribeQuorumResponseData.ReplicaState; +import org.apache.kafka.common.message.DescribeQuorumResponseData; import org.apache.kafka.common.message.EndQuorumEpochRequestData; import org.apache.kafka.common.message.EndQuorumEpochResponseData; import org.apache.kafka.common.message.FetchRequestData; import org.apache.kafka.common.message.FetchResponseData; -import org.apache.kafka.common.message.LeaderChangeMessage; +import org.apache.kafka.common.message.FetchSnapshotResponseData; import org.apache.kafka.common.message.LeaderChangeMessage.Voter; +import org.apache.kafka.common.message.LeaderChangeMessage; import org.apache.kafka.common.message.VoteRequestData; import org.apache.kafka.common.message.VoteResponseData; import org.apache.kafka.common.metrics.Metrics; @@ -45,6 +46,7 @@ import org.apache.kafka.common.requests.DescribeQuorumResponse; import org.apache.kafka.common.requests.EndQuorumEpochRequest; import org.apache.kafka.common.requests.EndQuorumEpochResponse; +import org.apache.kafka.common.requests.FetchSnapshotResponse; import org.apache.kafka.common.requests.VoteRequest; import org.apache.kafka.common.requests.VoteResponse; import org.apache.kafka.common.utils.LogContext; @@ -542,6 +544,19 @@ MemoryRecords assertSentFetchResponse( return (MemoryRecords) partitionResponse.recordSet(); } + Optional assertSentFetchSnapshotResponse(TopicPartition topicPartition) { + List sentMessages = channel.drainSentResponses(ApiKeys.FETCH_SNAPSHOT); + assertEquals(1, sentMessages.size()); + + RaftMessage message = sentMessages.get(0); + assertTrue(message.data() instanceof FetchSnapshotResponseData); + + FetchSnapshotResponseData response = (FetchSnapshotResponseData) message.data(); + assertEquals(Errors.NONE, Errors.forCode(response.errorCode())); + + return FetchSnapshotResponse.forTopicPartition(response, topicPartition); + } + void buildFollowerSet( int epoch, int closeFollower, From e64837768e79c3b78de2d5741cdbcbeb10b72737 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Tue, 3 Nov 2020 10:29:49 -0800 Subject: [PATCH 02/10] Implement FetchSnapshot response handling --- .../common/requests/FetchSnapshotRequest.java | 19 +- .../requests/FetchSnapshotResponse.java | 1 - .../common/message/FetchResponse.json | 7 + .../common/message/FetchSnapshotRequest.json | 6 +- .../org/apache/kafka/raft/CandidateState.java | 3 + .../org/apache/kafka/raft/EpochState.java | 3 +- .../org/apache/kafka/raft/FollowerState.java | 24 ++ .../apache/kafka/raft/KafkaRaftClient.java | 109 ++++++-- .../org/apache/kafka/raft/LeaderState.java | 3 + .../org/apache/kafka/raft/QuorumState.java | 4 + .../org/apache/kafka/raft/ResignedState.java | 3 + .../apache/kafka/raft/UnattachedState.java | 2 + .../org/apache/kafka/raft/VotedState.java | 2 + .../apache/kafka/snapshot/SnapshotWriter.java | 1 + .../raft/KafkaRaftClientSnapshotTest.java | 233 +++++++++++++++++- .../kafka/raft/KafkaRaftClientTest.java | 2 +- .../java/org/apache/kafka/raft/MockLog.java | 4 + .../kafka/raft/RaftClientTestContext.java | 9 +- .../kafka/snapshot/SnapshotWriterTest.java | 2 +- 19 files changed, 394 insertions(+), 43 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java index 2c4f195320e76..eacda14193edf 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java @@ -18,6 +18,7 @@ import java.util.Collections; import java.util.Optional; +import java.util.function.UnaryOperator; import org.apache.kafka.common.TopicPartition; import org.apache.kafka.common.message.FetchSnapshotRequestData; import org.apache.kafka.common.message.FetchSnapshotResponseData; @@ -46,24 +47,18 @@ public FetchSnapshotResponse getErrorResponse(int throttleTimeMs, Throwable e) { public static FetchSnapshotRequestData singleton( TopicPartition topicPartition, - FetchSnapshotRequestData.SnapshotId snapshotId, - int maxBytes, - long position + UnaryOperator operator ) { + FetchSnapshotRequestData.PartitionSnapshot partitionSnapshot = operator.apply( + new FetchSnapshotRequestData.PartitionSnapshot().setIndex(topicPartition.partition()) + ); + return new FetchSnapshotRequestData() - .setMaxBytes(maxBytes) .setTopics( Collections.singletonList( new FetchSnapshotRequestData.TopicSnapshot() .setName(topicPartition.topic()) - .setPartitions( - Collections.singletonList( - new FetchSnapshotRequestData.PartitionSnapshot() - .setIndex(topicPartition.partition()) - .setSnapshotId(snapshotId) - .setPosition(position) - ) - ) + .setPartitions(Collections.singletonList(partitionSnapshot)) ) ); } diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java index 878e15b4fb656..a6587746762ce 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java @@ -109,5 +109,4 @@ public static Optional forTopicPart .filter(parition -> parition.index() == topicPartition.partition()) .findAny(); } - } diff --git a/clients/src/main/resources/common/message/FetchResponse.json b/clients/src/main/resources/common/message/FetchResponse.json index 3db4359b3aaf2..0aa9a3a029e33 100644 --- a/clients/src/main/resources/common/message/FetchResponse.json +++ b/clients/src/main/resources/common/message/FetchResponse.json @@ -78,6 +78,13 @@ { "name": "LeaderEpoch", "type": "int32", "versions": "12+", "default": "-1", "about": "The latest known leader epoch"} ]}, + { "name": "SnapshotId", "type": "SnapshotId", + "versions": "12+", "taggedVersions": "12+", "tag": 2, + "about": "In the case of fetching an offset less than the LogStartOffset, this is the end offset and epoch that should be used in the FetchSnapshot request.", + "fields": [ + { "name": "EndOffset", "type": "int64", "versions": "0+", "default": "-1" }, + { "name": "Epoch", "type": "int32", "versions": "0+", "default": "-1" } + ]}, { "name": "AbortedTransactions", "type": "[]AbortedTransaction", "versions": "4+", "nullableVersions": "4+", "ignorable": true, "about": "The aborted transactions.", "fields": [ { "name": "ProducerId", "type": "int64", "versions": "4+", "entityType": "producerId", diff --git a/clients/src/main/resources/common/message/FetchSnapshotRequest.json b/clients/src/main/resources/common/message/FetchSnapshotRequest.json index 1467ed5114c94..630c18861ffa6 100644 --- a/clients/src/main/resources/common/message/FetchSnapshotRequest.json +++ b/clients/src/main/resources/common/message/FetchSnapshotRequest.json @@ -20,9 +20,11 @@ "validVersions": "0", "flexibleVersions": "0+", "fields": [ - { "name": "ReplicaId", "type": "int32", "versions": "0+", + { "name": "ClusterId", "type": "string", "versions": "0+", "nullableVersions": "0+", "default": "null", "taggedVersions": "0+", "tag": 0, + "about": "The clusterId if known. This is used to validate metadata fetches prior to broker registration." }, + { "name": "ReplicaId", "type": "int32", "versions": "0+", "default": "-1", "about": "The broker ID of the follower." }, - { "name": "MaxBytes", "type": "int32", "versions": "0+", + { "name": "MaxBytes", "type": "int32", "versions": "0+", "default": "0x7fffffff", "about": "The maximum bytes to fetch from all of the snapshots." }, { "name": "Topics", "type": "[]TopicSnapshot", "versions": "0+", "about": "The topics to fetch.", "fields": [ diff --git a/raft/src/main/java/org/apache/kafka/raft/CandidateState.java b/raft/src/main/java/org/apache/kafka/raft/CandidateState.java index eb58b32042b9a..08f69067995b2 100644 --- a/raft/src/main/java/org/apache/kafka/raft/CandidateState.java +++ b/raft/src/main/java/org/apache/kafka/raft/CandidateState.java @@ -250,6 +250,9 @@ public String name() { return "Candidate"; } + @Override + public void close() {} + private enum State { UNRECORDED, GRANTED, diff --git a/raft/src/main/java/org/apache/kafka/raft/EpochState.java b/raft/src/main/java/org/apache/kafka/raft/EpochState.java index 626657b2509bc..b32a200c5f77d 100644 --- a/raft/src/main/java/org/apache/kafka/raft/EpochState.java +++ b/raft/src/main/java/org/apache/kafka/raft/EpochState.java @@ -16,9 +16,10 @@ */ package org.apache.kafka.raft; +import java.io.Closeable; import java.util.Optional; -public interface EpochState { +public interface EpochState extends Closeable { default Optional highWatermark() { return Optional.empty(); diff --git a/raft/src/main/java/org/apache/kafka/raft/FollowerState.java b/raft/src/main/java/org/apache/kafka/raft/FollowerState.java index 5cbe45be437d0..6326fdd4f335f 100644 --- a/raft/src/main/java/org/apache/kafka/raft/FollowerState.java +++ b/raft/src/main/java/org/apache/kafka/raft/FollowerState.java @@ -18,7 +18,9 @@ import org.apache.kafka.common.utils.Time; import org.apache.kafka.common.utils.Timer; +import org.apache.kafka.snapshot.RawSnapshotWriter; +import java.io.IOException; import java.util.Optional; import java.util.OptionalInt; import java.util.OptionalLong; @@ -29,8 +31,11 @@ public class FollowerState implements EpochState { private final int epoch; private final int leaderId; private final Set voters; + // TODO: Document that this timer is for both Fetch and FetchSnapshot private final Timer fetchTimer; private Optional highWatermark; + // TODO: Document what this field is doing + private Optional fetchingSnapshot; public FollowerState( Time time, @@ -46,6 +51,7 @@ public FollowerState( this.voters = voters; this.fetchTimer = time.timer(fetchTimeoutMs); this.highWatermark = highWatermark; + this.fetchingSnapshot = Optional.empty(); } @Override @@ -120,6 +126,17 @@ public Optional highWatermark() { return highWatermark; } + public Optional fetchingSnapshot() { + return fetchingSnapshot; + } + + public void setFetchingSnapshot(Optional fetchingSnapshot) throws IOException { + if (fetchingSnapshot.isPresent()) { + fetchingSnapshot.get().close(); + } + this.fetchingSnapshot = fetchingSnapshot; + } + @Override public String toString() { return "FollowerState(" + @@ -129,4 +146,11 @@ public String toString() { ", voters=" + voters + ')'; } + + @Override + public void close() throws IOException { + if (fetchingSnapshot.isPresent()) { + fetchingSnapshot.get().close(); + } + } } diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index 6ea9b76c68160..8956cfece100d 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -67,6 +67,7 @@ import org.apache.kafka.raft.internals.RecordsBatchReader; import org.apache.kafka.raft.internals.ThresholdPurgatory; import org.apache.kafka.snapshot.RawSnapshotReader; +import org.apache.kafka.snapshot.RawSnapshotWriter; import org.apache.kafka.snapshot.SnapshotWriter; import org.slf4j.Logger; @@ -1044,6 +1045,14 @@ private boolean handleFetchResponse( logger.info("Truncated to offset {} from Fetch response from leader {}", truncationOffset, quorum.leaderIdOrNil()); }); + } else if (partitionResponse.snapshotId().epoch() >= 0 && partitionResponse.snapshotId().endOffset() >= 0) { + // The leader is asking us to fetch a snapshot + OffsetAndEpoch snapshotId = new OffsetAndEpoch( + partitionResponse.snapshotId().endOffset(), + partitionResponse.snapshotId().epoch() + ); + + state.setFetchingSnapshot(Optional.of(log.createSnapshot(snapshotId))); } else { Records records = (Records) partitionResponse.recordSet(); if (records.sizeInBytes() > 0) { @@ -1171,6 +1180,10 @@ private FetchSnapshotResponseData handleFetchSnapshotRequest( return FetchSnapshotResponse.singletonWithData( log.topicPartition(), responsePartitionSnapshot -> { + responsePartitionSnapshot.snapshotId() + .setEndOffset(snapshotId.offset) + .setEpoch(snapshotId.epoch); + return responsePartitionSnapshot .setSize(snapshotSize) .setPosition(partitionSnapshot.position()) @@ -1213,17 +1226,37 @@ private boolean handleFetchSnapshotResponse( return handled.get(); } - /* - // TODO: uncomment this block OffsetAndEpoch snapshotId = new OffsetAndEpoch( partitionSnapshot.snapshotId().endOffset(), partitionSnapshot.snapshotId().epoch() ); - */ + FollowerState state = quorum.followerStateOrThrow(); + RawSnapshotWriter snapshot; + if (state.fetchingSnapshot().isPresent()) { + snapshot = state.fetchingSnapshot().get(); + } else { + throw new IllegalStateException("Received unexpected fetch snapshot response: " + partitionSnapshot); + } - // TODO: handle the rest of the response + if (!snapshot.snapshotId().equals(snapshotId)) { + throw new IllegalStateException(String.format("Received fetch snapshot response with an invalid id. Expected %s; Received %s", snapshot.snapshotId(), snapshotId)); + } + if (snapshot.sizeInBytes() != partitionSnapshot.position()) { + throw new IllegalStateException(String.format("Received fetch snapshot response with an invalid position. Expected %s; Received %s", snapshot.sizeInBytes(), partitionSnapshot.position())); + } + + snapshot.append(partitionSnapshot.bytes()); + if (snapshot.sizeInBytes() == partitionSnapshot.size()) { + // Finished fetching the snapshot. + snapshot.freeze(); + state.setFetchingSnapshot(Optional.empty()); + + // TODO: Create a Jira: We need to update the log start offset and the log end offset + } + + state.resetFetchTimeout(currentTimeMs); return true; } @@ -1585,13 +1618,22 @@ private long maybeSendAnyVoterFetch(long currentTimeMs) { } } - private FetchSnapshotRequestData buildFetchSnapshotRequest() { - // TODO: fully implement this: how do we find out what snapshot we are suppose to fetch? - return new FetchSnapshotRequestData(); - } + private FetchSnapshotRequestData buildFetchSnapshotRequest(OffsetAndEpoch snapshotId, long snapshotSize) { + // TODO: Create a Jira. Set the cluster id on the request if we have it. + FetchSnapshotRequestData.SnapshotId requestSnapshotId = new FetchSnapshotRequestData.SnapshotId() + .setEpoch(snapshotId.epoch) + .setEndOffset(snapshotId.offset); + + FetchSnapshotRequestData request = FetchSnapshotRequest.singleton( + log.topicPartition(), + snapshotPartition -> { + return snapshotPartition + .setSnapshotId(requestSnapshotId) + .setPosition(snapshotSize); + } + ); - private void maybeSendFetchSnapshot(long currentTimeMs, int leaderId) throws IOException { - maybeSendRequest(currentTimeMs, leaderId, this::buildFetchSnapshotRequest); + return request.setReplicaId(quorum.localId); } public boolean isRunning() { @@ -1772,16 +1814,29 @@ private long pollFollowerAsVoter(FollowerState state, long currentTimeMs) throws transitionToCandidate(currentTimeMs); return 0L; } else { - long backoffMs = maybeSendRequest( - currentTimeMs, - state.leaderId(), - this::buildFetchRequest - ); + long backoffMs; + if (state.fetchingSnapshot().isPresent()) { + RawSnapshotWriter snapshot = state.fetchingSnapshot().get(); + long snapshotSize = snapshot.sizeInBytes(); + + backoffMs = maybeSendRequest( + currentTimeMs, + state.leaderId(), + () -> buildFetchSnapshotRequest(snapshot.snapshotId(), snapshotSize) + ); + } else { + backoffMs = maybeSendRequest( + currentTimeMs, + state.leaderId(), + this::buildFetchRequest + ); + } + return Math.min(backoffMs, state.remainingFetchTimeMs(currentTimeMs)); } } - private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) { + private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) throws IOException { if (state.hasFetchTimeoutExpired(currentTimeMs)) { return maybeSendAnyVoterFetch(currentTimeMs); } else { @@ -1797,12 +1852,24 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) { } else if (connection.isBackingOff(currentTimeMs)) { backoffMs = maybeSendAnyVoterFetch(currentTimeMs); } else { - backoffMs = maybeSendRequest( - currentTimeMs, - state.leaderId(), - this::buildFetchRequest - ); + if (state.fetchingSnapshot().isPresent()) { + RawSnapshotWriter snapshot = state.fetchingSnapshot().get(); + long snapshotSize = snapshot.sizeInBytes(); + + backoffMs = maybeSendRequest( + currentTimeMs, + state.leaderId(), + () -> buildFetchSnapshotRequest(snapshot.snapshotId(), snapshotSize) + ); + } else { + backoffMs = maybeSendRequest( + currentTimeMs, + state.leaderId(), + this::buildFetchRequest + ); + } } + return Math.min(backoffMs, state.remainingFetchTimeMs(currentTimeMs)); } } diff --git a/raft/src/main/java/org/apache/kafka/raft/LeaderState.java b/raft/src/main/java/org/apache/kafka/raft/LeaderState.java index 7b9d04f76f8d4..c1b45715c451a 100644 --- a/raft/src/main/java/org/apache/kafka/raft/LeaderState.java +++ b/raft/src/main/java/org/apache/kafka/raft/LeaderState.java @@ -287,4 +287,7 @@ public String name() { return "Leader"; } + @Override + public void close() {} + } diff --git a/raft/src/main/java/org/apache/kafka/raft/QuorumState.java b/raft/src/main/java/org/apache/kafka/raft/QuorumState.java index 3764308bddde8..9a5c761c15909 100644 --- a/raft/src/main/java/org/apache/kafka/raft/QuorumState.java +++ b/raft/src/main/java/org/apache/kafka/raft/QuorumState.java @@ -425,6 +425,10 @@ public void transitionToLeader(long epochStartOffset) throws IOException { } private void transitionTo(EpochState state) throws IOException { + if (this.state != null) { + this.state.close(); + } + this.store.writeElectionState(state.election()); this.state = state; log.info("Completed transition to {}", state); diff --git a/raft/src/main/java/org/apache/kafka/raft/ResignedState.java b/raft/src/main/java/org/apache/kafka/raft/ResignedState.java index 0dcb719a42e98..c1608aa9fc99e 100644 --- a/raft/src/main/java/org/apache/kafka/raft/ResignedState.java +++ b/raft/src/main/java/org/apache/kafka/raft/ResignedState.java @@ -141,4 +141,7 @@ public String toString() { ", preferredSuccessors=" + preferredSuccessors + ')'; } + + @Override + public void close() {} } diff --git a/raft/src/main/java/org/apache/kafka/raft/UnattachedState.java b/raft/src/main/java/org/apache/kafka/raft/UnattachedState.java index ca69f5940e627..62b82e232f17a 100644 --- a/raft/src/main/java/org/apache/kafka/raft/UnattachedState.java +++ b/raft/src/main/java/org/apache/kafka/raft/UnattachedState.java @@ -97,4 +97,6 @@ public String toString() { ')'; } + @Override + public void close() {} } diff --git a/raft/src/main/java/org/apache/kafka/raft/VotedState.java b/raft/src/main/java/org/apache/kafka/raft/VotedState.java index 5b47c3aa69c88..4138176b7c305 100644 --- a/raft/src/main/java/org/apache/kafka/raft/VotedState.java +++ b/raft/src/main/java/org/apache/kafka/raft/VotedState.java @@ -107,4 +107,6 @@ public String toString() { ')'; } + @Override + public void close() {} } diff --git a/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java b/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java index 0232e93ac380f..8a1a5fdcfaf9b 100644 --- a/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java +++ b/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java @@ -28,6 +28,7 @@ import org.apache.kafka.raft.internals.BatchAccumulator.CompletedBatch; import org.apache.kafka.raft.internals.BatchAccumulator; +// TODO: We should add a validate function that can be called before calling freeze /** * A type for writing a snapshot fora given end offset and epoch. * diff --git a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java index 0a5266fa5aad0..08f35236dcce6 100644 --- a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java +++ b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java @@ -20,17 +20,26 @@ import java.nio.ByteBuffer; import java.util.Arrays; import java.util.List; +import java.util.Optional; import java.util.Set; import org.apache.kafka.common.TopicPartition; +import org.apache.kafka.common.memory.MemoryPool; +import org.apache.kafka.common.message.FetchResponseData; import org.apache.kafka.common.message.FetchSnapshotRequestData; import org.apache.kafka.common.message.FetchSnapshotResponseData; import org.apache.kafka.common.protocol.Errors; +import org.apache.kafka.common.record.CompressionType; import org.apache.kafka.common.requests.FetchSnapshotRequest; +import org.apache.kafka.common.requests.FetchSnapshotResponse; import org.apache.kafka.common.utils.Utils; -import org.apache.kafka.snapshot.SnapshotWriter; +import org.apache.kafka.raft.internals.StringSerde; import org.apache.kafka.snapshot.RawSnapshotReader; +import org.apache.kafka.snapshot.RawSnapshotWriter; +import org.apache.kafka.snapshot.SnapshotWriter; +import org.apache.kafka.snapshot.SnapshotWriterTest; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; final public class KafkaRaftClientSnapshotTest { @Test @@ -124,7 +133,9 @@ public void testPartialFetchSnapshotRequestAsLeader() throws Exception { context.client.poll(); - FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + FetchSnapshotResponseData.PartitionSnapshot response = context + .assertSentFetchSnapshotResponse(context.metadataPartition) + .get(); assertEquals(Errors.NONE, Errors.forCode(response.errorCode())); assertEquals(snapshot.sizeInBytes(), response.size()); @@ -183,6 +194,69 @@ public void testFetchSnapshotRequestAsFollower() throws IOException { assertEquals(leaderId, response.currentLeader().leaderId()); } + @Test + public void testFetchResponseWithSnapshotId() throws Exception { + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, 2, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(100L, request.snapshotId().endOffset()); + assertEquals(1, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + List records = Arrays.asList("foo", "bar"); + MemorySnapshotWriter memorySnapshot = new MemorySnapshotWriter(snapshotId); + try (SnapshotWriter snapshotWriter = snapshotWriter(context, memorySnapshot)) { + snapshotWriter.append(records); + snapshotWriter.freeze(); + } + + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + fetchSnapshotResponse( + context.metadataPartition, + epoch, + leaderId, + snapshotId, + memorySnapshot.buffer().remaining(), + 0L, + memorySnapshot.buffer().slice() + ) + ); + + context.pollUntilSend(); + + try (RawSnapshotReader snapshot = context.log.readSnapshot(snapshotId).get()) { + assertEquals(memorySnapshot.buffer().remaining(), snapshot.sizeInBytes()); + SnapshotWriterTest.assertSnapshot(Arrays.asList(records), snapshot); + } + } + private static FetchSnapshotRequestData fetchSnapshotRequest( TopicPartition topicPartition, OffsetAndEpoch offsetAndEpoch, @@ -192,6 +266,159 @@ private static FetchSnapshotRequestData fetchSnapshotRequest( FetchSnapshotRequestData.SnapshotId snapshotId = new FetchSnapshotRequestData.SnapshotId() .setEndOffset(offsetAndEpoch.offset) .setEpoch(offsetAndEpoch.epoch); - return FetchSnapshotRequest.singleton(topicPartition, snapshotId, maxBytes, position); + + FetchSnapshotRequestData request = FetchSnapshotRequest.singleton( + topicPartition, + snapshotPartition -> { + return snapshotPartition + .setSnapshotId(snapshotId) + .setPosition(position); + } + ); + + return request.setMaxBytes(maxBytes); + } + + private static FetchSnapshotResponseData fetchSnapshotResponse( + TopicPartition topicPartition, + int leaderEpoch, + int leaderId, + OffsetAndEpoch snapshotId, + long size, + long position, + ByteBuffer buffer + ) { + return FetchSnapshotResponse.singletonWithData( + topicPartition, + partitionSnapshot -> { + partitionSnapshot.currentLeader() + .setLeaderEpoch(leaderEpoch) + .setLeaderId(leaderId); + + partitionSnapshot.snapshotId() + .setEndOffset(snapshotId.offset) + .setEpoch(snapshotId.epoch); + + return partitionSnapshot + .setSize(size) + .setPosition(position) + .setBytes(buffer); + } + ); + } + + private static FetchResponseData snapshotFetchResponse( + TopicPartition topicPartition, + int epoch, + int leaderId, + OffsetAndEpoch snapshotId, + long highWatermark + ) { + return RaftUtil.singletonFetchResponse(topicPartition, Errors.NONE, partitionData -> { + partitionData + .setErrorCode(Errors.NONE.code()) + .setHighWatermark(highWatermark); + + partitionData.currentLeader() + .setLeaderEpoch(epoch) + .setLeaderId(leaderId); + + partitionData.snapshotId() + .setEpoch(snapshotId.epoch) + .setEndOffset(snapshotId.offset); + }); + } + + private static Optional assertFetchSnapshotRequest( + RaftRequest.Outbound request, + TopicPartition topicPartition, + int replicaId, + int maxBytes + ) { + assertTrue(request.data() instanceof FetchSnapshotRequestData); + + FetchSnapshotRequestData data = (FetchSnapshotRequestData) request.data(); + + assertEquals(replicaId, data.replicaId()); + assertEquals(maxBytes, data.maxBytes()); + + return FetchSnapshotRequest.forTopicPartition(data, topicPartition); + } + + private static SnapshotWriter snapshotWriter(RaftClientTestContext context, RawSnapshotWriter snapshot) { + return new SnapshotWriter<>( + snapshot, + 4 * 1024, + MemoryPool.NONE, + context.time, + CompressionType.NONE, + new StringSerde() + ); + } + + private final static class MemorySnapshotWriter implements RawSnapshotWriter { + private final OffsetAndEpoch snapshotId; + private ByteBuffer data; + private boolean frozen; + + public MemorySnapshotWriter(OffsetAndEpoch snapshotId) { + this.snapshotId = snapshotId; + this.data = ByteBuffer.allocate(0); + this.frozen = false; + } + + @Override + public OffsetAndEpoch snapshotId() { + return snapshotId; + } + + @Override + public long sizeInBytes() { + if (frozen) { + throw new RuntimeException("Snapshot is already frozen " + snapshotId); + } + + return data.position(); + } + + @Override + public void append(ByteBuffer buffer) { + if (frozen) { + throw new RuntimeException("Snapshot is already frozen " + snapshotId); + } + + if (!(data.remaining() >= buffer.remaining())) { + ByteBuffer old = data; + old.flip(); + + int newSize = Math.max(data.capacity() * 2, data.capacity() + buffer.remaining()); + data = ByteBuffer.allocate(newSize); + + data.put(old); + } + data.put(buffer); + } + + @Override + public boolean isFrozen() { + return frozen; + } + + @Override + public void freeze() { + if (frozen) { + throw new RuntimeException("Snapshot is already frozen " + snapshotId); + } + + frozen = true; + data.flip(); + } + + @Override + public void close() {} + + public ByteBuffer buffer() { + return data; + } } } diff --git a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java index 126a380d397ee..c439b68bd3f80 100644 --- a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java +++ b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java @@ -1658,7 +1658,7 @@ public void testFollowerLogReconciliation() throws Exception { int correlationId = context.assertSentFetchRequest(epoch, 3L, lastEpoch); - FetchResponseData response = context.outOfRangeFetchRecordsResponse(epoch, otherNodeId, 2L, + FetchResponseData response = context.divergingFetchResponse(epoch, otherNodeId, 2L, lastEpoch, 1L); context.deliverResponse(correlationId, otherNodeId, response); diff --git a/raft/src/test/java/org/apache/kafka/raft/MockLog.java b/raft/src/test/java/org/apache/kafka/raft/MockLog.java index 1e5fbb092b07c..28eb0c54e9007 100644 --- a/raft/src/test/java/org/apache/kafka/raft/MockLog.java +++ b/raft/src/test/java/org/apache/kafka/raft/MockLog.java @@ -508,6 +508,10 @@ public OffsetAndEpoch snapshotId() { @Override public long sizeInBytes() { + if (frozen) { + throw new RuntimeException("Snapshot is already frozen " + snapshotId); + } + return data.position(); } diff --git a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java index 3d498ae7ca71b..f5d52466d526b 100644 --- a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java +++ b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java @@ -544,6 +544,13 @@ MemoryRecords assertSentFetchResponse( return (MemoryRecords) partitionResponse.recordSet(); } + RaftRequest.Outbound assertSentFetchSnapshotRequest() { + List sentRequests = channel.drainSentRequests(ApiKeys.FETCH_SNAPSHOT); + assertEquals(1, sentRequests.size()); + + return sentRequests.get(0); + } + Optional assertSentFetchSnapshotResponse(TopicPartition topicPartition) { List sentMessages = channel.drainSentResponses(ApiKeys.FETCH_SNAPSHOT); assertEquals(1, sentMessages.size()); @@ -807,7 +814,7 @@ FetchResponseData fetchResponse( }); } - FetchResponseData outOfRangeFetchRecordsResponse( + FetchResponseData divergingFetchResponse( int epoch, int leaderId, long divergingEpochEndOffset, diff --git a/raft/src/test/java/org/apache/kafka/snapshot/SnapshotWriterTest.java b/raft/src/test/java/org/apache/kafka/snapshot/SnapshotWriterTest.java index 3957640284e59..35652c75306c9 100644 --- a/raft/src/test/java/org/apache/kafka/snapshot/SnapshotWriterTest.java +++ b/raft/src/test/java/org/apache/kafka/snapshot/SnapshotWriterTest.java @@ -100,7 +100,7 @@ private List> buildRecords(int recordsPerBatch, int batches) { return result; } - private void assertSnapshot(List> batches, RawSnapshotReader reader) { + public static void assertSnapshot(List> batches, RawSnapshotReader reader) { List expected = new ArrayList<>(); batches.forEach(expected::addAll); From 4ea3c9b4fe4a1e2528d322291665d0365dbc304e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Wed, 2 Dec 2020 16:54:54 -0700 Subject: [PATCH 03/10] Include leader epoch in the FetchSnapshot request --- .../common/requests/FetchSnapshotRequest.java | 4 +- .../requests/FetchSnapshotResponse.java | 19 +- .../common/message/FetchSnapshotRequest.json | 20 +- .../apache/kafka/raft/KafkaRaftClient.java | 74 +++- .../raft/KafkaRaftClientSnapshotTest.java | 371 +++++++++++++++++- 5 files changed, 427 insertions(+), 61 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java index eacda14193edf..03ba072190c12 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java @@ -50,7 +50,7 @@ public static FetchSnapshotRequestData singleton( UnaryOperator operator ) { FetchSnapshotRequestData.PartitionSnapshot partitionSnapshot = operator.apply( - new FetchSnapshotRequestData.PartitionSnapshot().setIndex(topicPartition.partition()) + new FetchSnapshotRequestData.PartitionSnapshot().setPartition(topicPartition.partition()) ); return new FetchSnapshotRequestData() @@ -73,7 +73,7 @@ public static Optional forTopicParti .stream() .filter(topic -> topic.name().equals(topicPartition.topic())) .flatMap(topic -> topic.partitions().stream()) - .filter(parition -> parition.index() == topicPartition.partition()) + .filter(partition -> partition.partition() == topicPartition.partition()) .findAny(); } } diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java index a6587746762ce..69a9da7feafca 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java @@ -61,24 +61,7 @@ public static FetchSnapshotResponseData withTopError(Errors error) { return new FetchSnapshotResponseData().setErrorCode(error.code()); } - public static FetchSnapshotResponseData singletonWithError(TopicPartition topicPartition, Errors error) { - return new FetchSnapshotResponseData() - .setTopics( - Collections.singletonList( - new FetchSnapshotResponseData.TopicSnapshot() - .setName(topicPartition.topic()) - .setPartitions( - Collections.singletonList( - new FetchSnapshotResponseData.PartitionSnapshot() - .setIndex(topicPartition.partition()) - .setErrorCode(error.code()) - ) - ) - ) - ); - } - - public static FetchSnapshotResponseData singletonWithData( + public static FetchSnapshotResponseData singleton( TopicPartition topicPartition, UnaryOperator operator ) { diff --git a/clients/src/main/resources/common/message/FetchSnapshotRequest.json b/clients/src/main/resources/common/message/FetchSnapshotRequest.json index 630c18861ffa6..c3518f44765c7 100644 --- a/clients/src/main/resources/common/message/FetchSnapshotRequest.json +++ b/clients/src/main/resources/common/message/FetchSnapshotRequest.json @@ -21,19 +21,21 @@ "flexibleVersions": "0+", "fields": [ { "name": "ClusterId", "type": "string", "versions": "0+", "nullableVersions": "0+", "default": "null", "taggedVersions": "0+", "tag": 0, - "about": "The clusterId if known. This is used to validate metadata fetches prior to broker registration." }, + "about": "The clusterId if known, this is used to validate metadata fetches prior to broker registration" }, { "name": "ReplicaId", "type": "int32", "versions": "0+", "default": "-1", - "about": "The broker ID of the follower." }, + "about": "The broker ID of the follower" }, { "name": "MaxBytes", "type": "int32", "versions": "0+", "default": "0x7fffffff", - "about": "The maximum bytes to fetch from all of the snapshots." }, + "about": "The maximum bytes to fetch from all of the snapshots" }, { "name": "Topics", "type": "[]TopicSnapshot", "versions": "0+", - "about": "The topics to fetch.", "fields": [ + "about": "The topics to fetch", "fields": [ { "name": "Name", "type": "string", "versions": "0+", "entityType": "topicName", - "about": "The name of the topic to fetch." }, + "about": "The name of the topic to fetch" }, { "name": "Partitions", "type": "[]PartitionSnapshot", "versions": "0+", - "about": "The partitions to fetch.", "fields": [ - { "name": "Index", "type": "int32", "versions": "0+", - "about": "The partition index." }, + "about": "The partitions to fetch", "fields": [ + { "name": "Partition", "type": "int32", "versions": "0+", + "about": "The partition index" }, + { "name": "CurrentLeaderEpoch", "type": "int32", "versions": "0+", + "about": "The current leader epoch of the partition, -1 for unknown leader epoch" }, { "name": "SnapshotId", "type": "SnapshotId", "versions": "0+", "about": "The snapshot endOffset and epoch to fetch", "fields": [ @@ -41,7 +43,7 @@ { "name": "Epoch", "type": "int32", "versions": "0+" } ]}, { "name": "Position", "type": "int64", "versions": "0+", - "about": "The byte position within the snapshot to start fetching from." } + "about": "The byte position within the snapshot to start fetching from" } ]} ]} ] diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index 8956cfece100d..f4672bc1e5b27 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -949,6 +949,8 @@ private FetchResponseData tryCompleteFetchRequest( ) { Optional errorOpt = validateLeaderOnlyRequest(request.currentLeaderEpoch()); if (errorOpt.isPresent()) { + // TODO: The replica should return what information it knows about the current epoch and + // leader. return buildEmptyFetchResponse(errorOpt.get(), Optional.empty()); } @@ -1132,35 +1134,45 @@ private FetchSnapshotResponseData handleFetchSnapshotRequest( // The Raft client assumes that there is only one topic partition. TopicPartition unknownTopicPartition = new TopicPartition( data.topics().get(0).name(), - data.topics().get(0).partitions().get(0).index() + data.topics().get(0).partitions().get(0).partition() ); - return FetchSnapshotResponse.singletonWithError(unknownTopicPartition, Errors.UNKNOWN_TOPIC_OR_PARTITION); + return FetchSnapshotResponse.singleton( + unknownTopicPartition, + responsePartitionSnapshot -> { + return responsePartitionSnapshot + .setErrorCode(Errors.UNKNOWN_TOPIC_OR_PARTITION.code()); + } + ); } - if (!quorum.isLeader()) { - return FetchSnapshotResponse.singletonWithData( + FetchSnapshotRequestData.PartitionSnapshot partitionSnapshot = partitionSnapshotOpt.get(); + Optional leaderValidation = validateLeaderOnlyRequest( + partitionSnapshot.currentLeaderEpoch() + ); + if (leaderValidation.isPresent()) { + return FetchSnapshotResponse.singleton( log.topicPartition(), - partitionSnapshot -> { - partitionSnapshot.currentLeader() - .setLeaderEpoch(quorum.epoch()) - .setLeaderId(quorum.leaderIdOrNil()); - - return partitionSnapshot; + responsePartitionSnapshot -> { + return addQuorumLeader(responsePartitionSnapshot) + .setErrorCode(leaderValidation.get().code()); } ); } - FetchSnapshotRequestData.PartitionSnapshot partitionSnapshot = partitionSnapshotOpt.get(); OffsetAndEpoch snapshotId = new OffsetAndEpoch( partitionSnapshot.snapshotId().endOffset(), partitionSnapshot.snapshotId().epoch() ); - - Optional snapshotOpt = log.readSnapshot(snapshotId); if (!snapshotOpt.isPresent()) { - return FetchSnapshotResponse.singletonWithError(log.topicPartition(), Errors.SNAPSHOT_NOT_FOUND); + return FetchSnapshotResponse.singleton( + log.topicPartition(), + responsePartitionSnapshot -> { + return addQuorumLeader(responsePartitionSnapshot) + .setErrorCode(Errors.SNAPSHOT_NOT_FOUND.code()); + } + ); } try (RawSnapshotReader snapshot = snapshotOpt.get()) { @@ -1170,6 +1182,7 @@ private FetchSnapshotResponseData handleFetchSnapshotRequest( } catch (ArithmeticException e) { maxSnapshotSize = Integer.MAX_VALUE; } + // TODO: Make sure that we also limit based on the fetch max bytes configuration ByteBuffer buffer = ByteBuffer.allocate(Math.min(data.maxBytes(), maxSnapshotSize)); snapshot.read(buffer, partitionSnapshot.position()); @@ -1177,10 +1190,11 @@ private FetchSnapshotResponseData handleFetchSnapshotRequest( long snapshotSize = snapshot.sizeInBytes(); - return FetchSnapshotResponse.singletonWithData( + return FetchSnapshotResponse.singleton( log.topicPartition(), responsePartitionSnapshot -> { - responsePartitionSnapshot.snapshotId() + addQuorumLeader(responsePartitionSnapshot) + .snapshotId() .setEndOffset(snapshotId.offset) .setEpoch(snapshotId.epoch); @@ -1200,6 +1214,7 @@ private boolean handleFetchSnapshotResponse( FetchSnapshotResponseData data = (FetchSnapshotResponseData) responseMetadata.data; Errors topLevelError = Errors.forCode(data.errorCode()); if (topLevelError != Errors.NONE) { + // TODO: check what values this expression returns return handleTopLevelError(topLevelError, responseMetadata); } @@ -1223,15 +1238,29 @@ private boolean handleFetchSnapshotResponse( Optional handled = maybeHandleCommonResponse( error, responseLeaderId, responseEpoch, currentTimeMs); if (handled.isPresent()) { + // TODO: check what values this expression returns return handled.get(); } + FollowerState state = quorum.followerStateOrThrow(); + + if (Errors.forCode(partitionSnapshot.errorCode()) == Errors.SNAPSHOT_NOT_FOUND || + partitionSnapshot.snapshotId().endOffset() < 0 || + partitionSnapshot.snapshotId().epoch() < 0) { + + /* The leader deleted the snapshot before the follower could download it. Start over by + * reseting the fetching snapshot state and sending another fetch request. + */ + state.setFetchingSnapshot(Optional.empty()); + state.resetFetchTimeout(currentTimeMs); + return true; + } + OffsetAndEpoch snapshotId = new OffsetAndEpoch( partitionSnapshot.snapshotId().endOffset(), partitionSnapshot.snapshotId().epoch() ); - FollowerState state = quorum.followerStateOrThrow(); RawSnapshotWriter snapshot; if (state.fetchingSnapshot().isPresent()) { snapshot = state.fetchingSnapshot().get(); @@ -1628,6 +1657,7 @@ private FetchSnapshotRequestData buildFetchSnapshotRequest(OffsetAndEpoch snapsh log.topicPartition(), snapshotPartition -> { return snapshotPartition + .setCurrentLeaderEpoch(quorum.epoch()) .setSnapshotId(requestSnapshotId) .setPosition(snapshotSize); } @@ -1636,6 +1666,16 @@ private FetchSnapshotRequestData buildFetchSnapshotRequest(OffsetAndEpoch snapsh return request.setReplicaId(quorum.localId); } + private FetchSnapshotResponseData.PartitionSnapshot addQuorumLeader( + FetchSnapshotResponseData.PartitionSnapshot partitionSnapshot + ) { + partitionSnapshot.currentLeader() + .setLeaderEpoch(quorum.epoch()) + .setLeaderId(quorum.leaderIdOrNil()); + + return partitionSnapshot; + } + public boolean isRunning() { GracefulShutdown gracefulShutdown = shutdown.get(); return gracefulShutdown == null || !gracefulShutdown.isFinished(); diff --git a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java index 08f35236dcce6..e8c375ea2f252 100644 --- a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java +++ b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java @@ -38,6 +38,7 @@ import org.apache.kafka.snapshot.SnapshotWriter; import org.apache.kafka.snapshot.SnapshotWriterTest; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Disabled; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -50,7 +51,15 @@ public void testMissingFetchSnapshotRequest() throws Exception { RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); - context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, new OffsetAndEpoch(0, 0), Integer.MAX_VALUE, 0)); + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch, + new OffsetAndEpoch(0, 0), + Integer.MAX_VALUE, + 0 + ) + ); context.client.poll(); @@ -67,7 +76,15 @@ public void testUnknownFetchSnapshotRequest() throws Exception { RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); - context.deliverRequest(fetchSnapshotRequest(topicPartition, new OffsetAndEpoch(0, 0), Integer.MAX_VALUE, 0)); + context.deliverRequest( + fetchSnapshotRequest( + topicPartition, + epoch, + new OffsetAndEpoch(0, 0), + Integer.MAX_VALUE, + 0 + ) + ); context.client.poll(); @@ -91,7 +108,15 @@ public void testFetchSnapshotRequestAsLeader() throws Exception { } try (RawSnapshotReader snapshot = context.log.readSnapshot(snapshotId).get()) { - context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, snapshotId, Integer.MAX_VALUE, 0)); + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch, + snapshotId, + Integer.MAX_VALUE, + 0 + ) + ); context.client.poll(); @@ -104,7 +129,7 @@ public void testFetchSnapshotRequestAsLeader() throws Exception { assertEquals(0, response.position()); assertEquals(snapshot.sizeInBytes(), response.bytes().remaining()); - ByteBuffer buffer = ByteBuffer.allocate((int) snapshot.sizeInBytes()); + ByteBuffer buffer = ByteBuffer.allocate(Math.toIntExact(snapshot.sizeInBytes())); snapshot.read(buffer, 0); buffer.flip(); @@ -129,7 +154,15 @@ public void testPartialFetchSnapshotRequestAsLeader() throws Exception { try (RawSnapshotReader snapshot = context.log.readSnapshot(snapshotId).get()) { // Fetch half of the snapshot - context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, snapshotId, (int) snapshot.sizeInBytes() / 2, 0)); + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch, + snapshotId, + Math.toIntExact(snapshot.sizeInBytes() / 2), + 0 + ) + ); context.client.poll(); @@ -142,21 +175,27 @@ public void testPartialFetchSnapshotRequestAsLeader() throws Exception { assertEquals(0, response.position()); assertEquals(snapshot.sizeInBytes() / 2, response.bytes().remaining()); - ByteBuffer snapshotBuffer = ByteBuffer.allocate((int) snapshot.sizeInBytes()); + ByteBuffer snapshotBuffer = ByteBuffer.allocate(Math.toIntExact(snapshot.sizeInBytes())); snapshot.read(snapshotBuffer, 0); snapshotBuffer.flip(); - ByteBuffer responseBuffer = ByteBuffer.allocate((int) snapshot.sizeInBytes()); + ByteBuffer responseBuffer = ByteBuffer.allocate(Math.toIntExact(snapshot.sizeInBytes())); responseBuffer.put(response.bytes()); ByteBuffer expectedBytes = snapshotBuffer.duplicate(); - expectedBytes.limit((int) snapshot.sizeInBytes() / 2); + expectedBytes.limit(Math.toIntExact(snapshot.sizeInBytes() / 2)); assertEquals(expectedBytes, responseBuffer.duplicate().flip()); // Fetch the remainder of the snapshot context.deliverRequest( - fetchSnapshotRequest(context.metadataPartition, snapshotId, Integer.MAX_VALUE, responseBuffer.position()) + fetchSnapshotRequest( + context.metadataPartition, + epoch, + snapshotId, + Integer.MAX_VALUE, + responseBuffer.position() + ) ); context.client.poll(); @@ -184,16 +223,36 @@ public void testFetchSnapshotRequestAsFollower() throws IOException { .withElectedLeader(epoch, leaderId) .build(); - context.deliverRequest(fetchSnapshotRequest(context.metadataPartition, snapshotId, Integer.MAX_VALUE, 0)); + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch, + snapshotId, + Integer.MAX_VALUE, + 0 + ) + ); context.client.poll(); FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); - assertEquals(Errors.NONE, Errors.forCode(response.errorCode())); + assertEquals(Errors.NOT_LEADER_OR_FOLLOWER, Errors.forCode(response.errorCode())); assertEquals(epoch, response.currentLeader().leaderEpoch()); assertEquals(leaderId, response.currentLeader().leaderId()); } + @Disabled + @Test + public void testFetchSnapshotRequestWithOlderEpoch() throws IOException { + assertTrue(false); + } + + @Disabled + @Test + public void testFetchSnapshotRequestWithNewerEpoch() throws IOException { + assertTrue(false); + } + @Test public void testFetchResponseWithSnapshotId() throws Exception { int localId = 0; @@ -208,7 +267,7 @@ public void testFetchResponseWithSnapshotId() throws Exception { context.pollUntilSend(); RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); - context.assertFetchRequestData(fetchRequest, 2, 0L, 0); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); context.deliverResponse( fetchRequest.correlationId, @@ -224,8 +283,8 @@ public void testFetchResponseWithSnapshotId() throws Exception { localId, Integer.MAX_VALUE ).get(); - assertEquals(100L, request.snapshotId().endOffset()); - assertEquals(1, request.snapshotId().epoch()); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); assertEquals(0, request.position()); List records = Arrays.asList("foo", "bar"); @@ -257,8 +316,289 @@ public void testFetchResponseWithSnapshotId() throws Exception { } } + @Test + public void testFetchSnapshotResponsePartialData() throws Exception { + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + List records = Arrays.asList("foo", "bar"); + MemorySnapshotWriter memorySnapshot = new MemorySnapshotWriter(snapshotId); + try (SnapshotWriter snapshotWriter = snapshotWriter(context, memorySnapshot)) { + snapshotWriter.append(records); + snapshotWriter.freeze(); + } + + ByteBuffer sendingBuffer = memorySnapshot.buffer().slice(); + sendingBuffer.limit(sendingBuffer.limit() / 2); + + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + fetchSnapshotResponse( + context.metadataPartition, + epoch, + leaderId, + snapshotId, + memorySnapshot.buffer().remaining(), + 0L, + sendingBuffer + ) + ); + + context.pollUntilSend(); + snapshotRequest = context.assertSentFetchSnapshotRequest(); + request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(sendingBuffer.limit(), request.position()); + + sendingBuffer = memorySnapshot.buffer().slice(); + sendingBuffer.position(Math.toIntExact(request.position())); + + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + fetchSnapshotResponse( + context.metadataPartition, + epoch, + leaderId, + snapshotId, + memorySnapshot.buffer().remaining(), + request.position(), + sendingBuffer + ) + ); + + context.pollUntilSend(); + + try (RawSnapshotReader snapshot = context.log.readSnapshot(snapshotId).get()) { + assertEquals(memorySnapshot.buffer().remaining(), snapshot.sizeInBytes()); + SnapshotWriterTest.assertSnapshot(Arrays.asList(records), snapshot); + } + } + + @Test + public void testFetchSnapshotResponseMissingSnapshot() throws Exception { + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + // Reply with a snapshot not found error + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + FetchSnapshotResponse.singleton( + context.metadataPartition, + responsePartitionSnapshot -> { + responsePartitionSnapshot + .currentLeader() + .setLeaderEpoch(epoch) + .setLeaderId(leaderId); + + return responsePartitionSnapshot + .setErrorCode(Errors.SNAPSHOT_NOT_FOUND.code()); + } + ) + ); + + context.pollUntilSend(); + fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + } + + @Test + public void testFetchSnapshotResponseFromNewerEpochNotLeader() throws Exception { + int localId = 0; + int firstLeaderId = localId + 1; + int secondLeaderId = firstLeaderId + 1; + Set voters = Utils.mkSet(localId, firstLeaderId, secondLeaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, firstLeaderId) + .build(); + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, firstLeaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + // Reply with new leader response + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + FetchSnapshotResponse.singleton( + context.metadataPartition, + responsePartitionSnapshot -> { + responsePartitionSnapshot + .currentLeader() + .setLeaderEpoch(epoch + 1) + .setLeaderId(secondLeaderId); + + return responsePartitionSnapshot + .setErrorCode(Errors.FENCED_LEADER_EPOCH.code()); + } + ) + ); + + context.pollUntilSend(); + fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch + 1, 0L, 0); + } + + @Test + public void testFetchSnapshotResponseFromNewerEpochLeader() throws Exception { + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + // Reply with new leader epoch + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + FetchSnapshotResponse.singleton( + context.metadataPartition, + responsePartitionSnapshot -> { + responsePartitionSnapshot + .currentLeader() + .setLeaderEpoch(epoch + 1) + .setLeaderId(leaderId); + + return responsePartitionSnapshot + .setErrorCode(Errors.FENCED_LEADER_EPOCH.code()); + } + ) + ); + + context.pollUntilSend(); + fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch + 1, 0L, 0); + } + + @Disabled + @Test + public void testFetchSnapshotResponseFromOlderEpoch() throws Exception { + assertTrue(false); + } + + + @Disabled + @Test + public void testFetchSnapshotResponseToNotFollower() throws Exception { + assertTrue(false); + } + private static FetchSnapshotRequestData fetchSnapshotRequest( TopicPartition topicPartition, + int epoch, OffsetAndEpoch offsetAndEpoch, int maxBytes, long position @@ -271,6 +611,7 @@ private static FetchSnapshotRequestData fetchSnapshotRequest( topicPartition, snapshotPartition -> { return snapshotPartition + .setCurrentLeaderEpoch(epoch) .setSnapshotId(snapshotId) .setPosition(position); } @@ -288,7 +629,7 @@ private static FetchSnapshotResponseData fetchSnapshotResponse( long position, ByteBuffer buffer ) { - return FetchSnapshotResponse.singletonWithData( + return FetchSnapshotResponse.singleton( topicPartition, partitionSnapshot -> { partitionSnapshot.currentLeader() From 460992277664c781c08627514d8391bbad8cf5af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Mon, 7 Dec 2020 19:39:40 -0700 Subject: [PATCH 04/10] Fix FetchSnapsot request and response class --- .../common/requests/FetchSnapshotRequest.java | 12 +++++------ .../requests/FetchSnapshotResponse.java | 20 +++++++++++++------ .../java/org/apache/kafka/raft/MockLog.java | 1 - 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java index 03ba072190c12..b9614ec8cdf0f 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java @@ -24,7 +24,7 @@ import org.apache.kafka.common.message.FetchSnapshotResponseData; import org.apache.kafka.common.protocol.ApiKeys; import org.apache.kafka.common.protocol.Errors; -import org.apache.kafka.common.protocol.types.Struct; +import org.apache.kafka.common.protocol.Message; final public class FetchSnapshotRequest extends AbstractRequest { public final FetchSnapshotRequestData data; @@ -34,17 +34,17 @@ public FetchSnapshotRequest(FetchSnapshotRequestData data) { this.data = data; } - @Override - protected Struct toStruct() { - return data.toStruct(version()); - } - @Override public FetchSnapshotResponse getErrorResponse(int throttleTimeMs, Throwable e) { // TODO: we need to handle throttleTimeMs return new FetchSnapshotResponse(new FetchSnapshotResponseData().setErrorCode(Errors.forException(e).code())); } + @Override + protected Message data() { + return data; + } + public static FetchSnapshotRequestData singleton( TopicPartition topicPartition, UnaryOperator operator diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java index 69a9da7feafca..b6b15b8daf874 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java @@ -23,19 +23,17 @@ import java.util.function.UnaryOperator; import org.apache.kafka.common.TopicPartition; import org.apache.kafka.common.message.FetchSnapshotResponseData; +import org.apache.kafka.common.protocol.ApiKeys; import org.apache.kafka.common.protocol.Errors; -import org.apache.kafka.common.protocol.types.Struct; +import org.apache.kafka.common.protocol.Message; final public class FetchSnapshotResponse extends AbstractResponse { public final FetchSnapshotResponseData data; public FetchSnapshotResponse(FetchSnapshotResponseData data) { - this.data = data; - } + super(ApiKeys.FETCH_SNAPSHOT); - @Override - protected Struct toStruct(short version) { - return data.toStruct(version); + this.data = data; } @Override @@ -57,6 +55,16 @@ public Map errorCounts() { return errors; } + @Override + public int throttleTimeMs() { + return data.throttleTimeMs(); + } + + @Override + protected Message data() { + return data; + } + public static FetchSnapshotResponseData withTopError(Errors error) { return new FetchSnapshotResponseData().setErrorCode(error.code()); } diff --git a/raft/src/test/java/org/apache/kafka/raft/MockLog.java b/raft/src/test/java/org/apache/kafka/raft/MockLog.java index 28eb0c54e9007..0f72125bb39ea 100644 --- a/raft/src/test/java/org/apache/kafka/raft/MockLog.java +++ b/raft/src/test/java/org/apache/kafka/raft/MockLog.java @@ -21,7 +21,6 @@ import org.apache.kafka.common.record.CompressionType; import org.apache.kafka.common.record.MemoryRecords; import org.apache.kafka.common.record.MemoryRecordsBuilder; -import org.apache.kafka.common.record.MutableRecordBatch; import org.apache.kafka.common.record.Record; import org.apache.kafka.common.record.RecordBatch; import org.apache.kafka.common.record.Records; From a96d16abfbd21a424fc3da78fbf69b6dc1630bac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Mon, 7 Dec 2020 20:06:43 -0700 Subject: [PATCH 05/10] Implement better snapshot id validation in fetch response --- .../apache/kafka/raft/KafkaRaftClient.java | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index f4672bc1e5b27..f026b7872666e 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -1047,8 +1047,29 @@ private boolean handleFetchResponse( logger.info("Truncated to offset {} from Fetch response from leader {}", truncationOffset, quorum.leaderIdOrNil()); }); - } else if (partitionResponse.snapshotId().epoch() >= 0 && partitionResponse.snapshotId().endOffset() >= 0) { + } else if (partitionResponse.snapshotId().epoch() >= 0 || + partitionResponse.snapshotId().endOffset() >= 0) { // The leader is asking us to fetch a snapshot + + if (partitionResponse.snapshotId().epoch() < 0) { + throw new KafkaException( + String.format( + "The leader sent a snapshot id with a valid end offset %s but with an invalid epoch %s", + partitionResponse.snapshotId().endOffset(), + partitionResponse.snapshotId().epoch() + ) + ); + } + if (partitionResponse.snapshotId().endOffset() < 0) { + throw new KafkaException( + String.format( + "The leader sent a snapshot id with a valid epoch %s but with an invalid end offset %s", + partitionResponse.snapshotId().epoch(), + partitionResponse.snapshotId().endOffset() + ) + ); + } + OffsetAndEpoch snapshotId = new OffsetAndEpoch( partitionResponse.snapshotId().endOffset(), partitionResponse.snapshotId().epoch() From 79fed6115ce20aedee3cea0cc9a4a8c5ba1bcca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Mon, 7 Dec 2020 23:05:17 -0700 Subject: [PATCH 06/10] Improve documentation --- .../common/requests/FetchSnapshotRequest.java | 24 ++++++++++++++++--- .../requests/FetchSnapshotResponse.java | 23 +++++++++++++++++- .../org/apache/kafka/raft/FollowerState.java | 6 +++-- .../apache/kafka/raft/KafkaRaftClient.java | 3 --- .../apache/kafka/snapshot/SnapshotWriter.java | 1 - 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java index b9614ec8cdf0f..ecc2ce5c93636 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java @@ -36,8 +36,11 @@ public FetchSnapshotRequest(FetchSnapshotRequestData data) { @Override public FetchSnapshotResponse getErrorResponse(int throttleTimeMs, Throwable e) { - // TODO: we need to handle throttleTimeMs - return new FetchSnapshotResponse(new FetchSnapshotResponseData().setErrorCode(Errors.forException(e).code())); + return new FetchSnapshotResponse( + new FetchSnapshotResponseData() + .setThrottleTimeMs(throttleTimeMs) + .setErrorCode(Errors.forException(e).code()) + ); } @Override @@ -45,6 +48,15 @@ protected Message data() { return data; } + /** + * Creates a FetchSnapshotRequestData with a single PartitionSnapshot for the topic partition. + * + * The partition index will already be populated when calling operator. + * + * @param topicPartition the topic partition to include + * @param operator unary operator responsible for populating all the appropriate fields + * @return the created fetch snapshot request data + */ public static FetchSnapshotRequestData singleton( TopicPartition topicPartition, UnaryOperator operator @@ -63,7 +75,13 @@ public static FetchSnapshotRequestData singleton( ); } - // TODO: write documentation. This function assumes that topic partitions are unique in `data` + /** + * Finds the PartitionSnapshot for a given topic partition. + * + * @param data the fetch snapshot request data + * @param topicPartition the topic partition to find + * @return the request partition snapshot if found, otherwise an empty Optional + */ public static Optional forTopicPartition( FetchSnapshotRequestData data, TopicPartition topicPartition diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java index b6b15b8daf874..de0dab1d9ee0d 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java @@ -65,10 +65,25 @@ protected Message data() { return data; } + /** + * Creates a FetchSnapshotResponseData with a top level error. + * + * @param error the top level error + * @return the created fetch snapshot response data + */ public static FetchSnapshotResponseData withTopError(Errors error) { return new FetchSnapshotResponseData().setErrorCode(error.code()); } + /** + * Creates a FetchSnapshotResponseData with a single PartitionSnapshot for the topic partition. + * + * The partition index will already by populated when calling operator. + * + * @param topicPartition the topic partition to include + * @param operator unary operator responsible for populating all of the appropriate fields + * @return the created fetch snapshot response data + */ public static FetchSnapshotResponseData singleton( TopicPartition topicPartition, UnaryOperator operator @@ -87,7 +102,13 @@ public static FetchSnapshotResponseData singleton( ); } - // TODO: write documentation. This function assumes that topic partitions are unique in `data` + /** + * Finds the PartitionSnapshot for a given topic partition. + * + * @param data the fetch snapshot response data + * @param topicPartition the topic partition to find + * @return the response partition snapshot if found, otherwise an empty Optional + */ public static Optional forTopicPartition( FetchSnapshotResponseData data, TopicPartition topicPartition diff --git a/raft/src/main/java/org/apache/kafka/raft/FollowerState.java b/raft/src/main/java/org/apache/kafka/raft/FollowerState.java index 6326fdd4f335f..e1ef7aa6d56be 100644 --- a/raft/src/main/java/org/apache/kafka/raft/FollowerState.java +++ b/raft/src/main/java/org/apache/kafka/raft/FollowerState.java @@ -31,10 +31,12 @@ public class FollowerState implements EpochState { private final int epoch; private final int leaderId; private final Set voters; - // TODO: Document that this timer is for both Fetch and FetchSnapshot + // Used for tracking the expiration of both the Fetch and FetchSnapshot requests private final Timer fetchTimer; private Optional highWatermark; - // TODO: Document what this field is doing + /* Used to track the currently fetching snapshot. When fetching snapshot regular + * Fetch request are paused + */ private Optional fetchingSnapshot; public FollowerState( diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index f026b7872666e..f5cb42a6893fc 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -1302,8 +1302,6 @@ private boolean handleFetchSnapshotResponse( // Finished fetching the snapshot. snapshot.freeze(); state.setFetchingSnapshot(Optional.empty()); - - // TODO: Create a Jira: We need to update the log start offset and the log end offset } state.resetFetchTimeout(currentTimeMs); @@ -1669,7 +1667,6 @@ private long maybeSendAnyVoterFetch(long currentTimeMs) { } private FetchSnapshotRequestData buildFetchSnapshotRequest(OffsetAndEpoch snapshotId, long snapshotSize) { - // TODO: Create a Jira. Set the cluster id on the request if we have it. FetchSnapshotRequestData.SnapshotId requestSnapshotId = new FetchSnapshotRequestData.SnapshotId() .setEpoch(snapshotId.epoch) .setEndOffset(snapshotId.offset); diff --git a/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java b/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java index 8a1a5fdcfaf9b..0232e93ac380f 100644 --- a/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java +++ b/raft/src/main/java/org/apache/kafka/snapshot/SnapshotWriter.java @@ -28,7 +28,6 @@ import org.apache.kafka.raft.internals.BatchAccumulator.CompletedBatch; import org.apache.kafka.raft.internals.BatchAccumulator; -// TODO: We should add a validate function that can be called before calling freeze /** * A type for writing a snapshot fora given end offset and epoch. * From 33be3a4be9b8f558e1d0cd0dadc8c98ee82eeb26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Tue, 22 Dec 2020 12:15:38 -0800 Subject: [PATCH 07/10] Add position out of range error --- .../errors/PositionOutOfRangeException.java | 31 +++++ .../apache/kafka/common/protocol/Errors.java | 13 ++- .../requests/FetchSnapshotResponse.java | 2 +- .../apache/kafka/raft/KafkaRaftClient.java | 109 ++++++++---------- .../raft/KafkaRaftClientSnapshotTest.java | 6 + 5 files changed, 95 insertions(+), 66 deletions(-) create mode 100644 clients/src/main/java/org/apache/kafka/common/errors/PositionOutOfRangeException.java diff --git a/clients/src/main/java/org/apache/kafka/common/errors/PositionOutOfRangeException.java b/clients/src/main/java/org/apache/kafka/common/errors/PositionOutOfRangeException.java new file mode 100644 index 0000000000000..c502d19498580 --- /dev/null +++ b/clients/src/main/java/org/apache/kafka/common/errors/PositionOutOfRangeException.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.common.errors; + +public class PositionOutOfRangeException extends ApiException { + + private static final long serialVersionUID = 1; + + public PositionOutOfRangeException(String s) { + super(s); + } + + public PositionOutOfRangeException(String message, Throwable cause) { + super(message, cause); + } + +} diff --git a/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java b/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java index 77498c4f2ee41..265b3bc091f4b 100644 --- a/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java +++ b/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java @@ -34,10 +34,9 @@ import org.apache.kafka.common.errors.DuplicateSequenceException; import org.apache.kafka.common.errors.ElectionNotNeededException; import org.apache.kafka.common.errors.EligibleLeadersNotAvailableException; -import org.apache.kafka.common.errors.FencedInstanceIdException; import org.apache.kafka.common.errors.FeatureUpdateFailedException; +import org.apache.kafka.common.errors.FencedInstanceIdException; import org.apache.kafka.common.errors.FencedLeaderEpochException; -import org.apache.kafka.common.errors.InvalidUpdateVersionException; import org.apache.kafka.common.errors.FetchSessionIdNotFoundException; import org.apache.kafka.common.errors.GroupAuthorizationException; import org.apache.kafka.common.errors.GroupIdNotFoundException; @@ -56,6 +55,7 @@ import org.apache.kafka.common.errors.InvalidPartitionsException; import org.apache.kafka.common.errors.InvalidPidMappingException; import org.apache.kafka.common.errors.InvalidPrincipalTypeException; +import org.apache.kafka.common.errors.InvalidProducerEpochException; import org.apache.kafka.common.errors.InvalidReplicaAssignmentException; import org.apache.kafka.common.errors.InvalidReplicationFactorException; import org.apache.kafka.common.errors.InvalidRequestException; @@ -65,6 +65,7 @@ import org.apache.kafka.common.errors.InvalidTopicException; import org.apache.kafka.common.errors.InvalidTxnStateException; import org.apache.kafka.common.errors.InvalidTxnTimeoutException; +import org.apache.kafka.common.errors.InvalidUpdateVersionException; import org.apache.kafka.common.errors.KafkaStorageException; import org.apache.kafka.common.errors.LeaderNotAvailableException; import org.apache.kafka.common.errors.ListenerNotFoundException; @@ -83,6 +84,7 @@ import org.apache.kafka.common.errors.OperationNotAttemptedException; import org.apache.kafka.common.errors.OutOfOrderSequenceException; import org.apache.kafka.common.errors.PolicyViolationException; +import org.apache.kafka.common.errors.PositionOutOfRangeException; import org.apache.kafka.common.errors.PreferredLeaderNotAvailableException; import org.apache.kafka.common.errors.PrincipalDeserializationException; import org.apache.kafka.common.errors.ProducerFencedException; @@ -116,7 +118,6 @@ import org.apache.kafka.common.errors.UnsupportedForMessageFormatException; import org.apache.kafka.common.errors.UnsupportedSaslMechanismException; import org.apache.kafka.common.errors.UnsupportedVersionException; -import org.apache.kafka.common.errors.InvalidProducerEpochException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -343,7 +344,11 @@ public enum Errors { FEATURE_UPDATE_FAILED(96, "Unable to update finalized features due to an unexpected server error.", FeatureUpdateFailedException::new), PRINCIPAL_DESERIALIZATION_FAILURE(97, "Request principal deserialization failed during forwarding. " + "This indicates an internal error on the broker cluster security setup.", PrincipalDeserializationException::new), - SNAPSHOT_NOT_FOUND(98, "Requested snapshot was not found", SnapshotNotFoundException::new); + SNAPSHOT_NOT_FOUND(98, "Requested snapshot was not found", SnapshotNotFoundException::new), + POSITION_OUT_OF_RANGE( + 99, + "Requested position is not greater than or equal to zero, and less than the size of the snapshot.", + PositionOutOfRangeException::new); private static final Logger log = LoggerFactory.getLogger(Errors.class); diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java index de0dab1d9ee0d..b503292f5e625 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java @@ -71,7 +71,7 @@ protected Message data() { * @param error the top level error * @return the created fetch snapshot response data */ - public static FetchSnapshotResponseData withTopError(Errors error) { + public static FetchSnapshotResponseData withTopLevelError(Errors error) { return new FetchSnapshotResponseData().setErrorCode(error.code()); } diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index f5cb42a6893fc..5b29ef9178162 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -949,8 +949,6 @@ private FetchResponseData tryCompleteFetchRequest( ) { Optional errorOpt = validateLeaderOnlyRequest(request.currentLeaderEpoch()); if (errorOpt.isPresent()) { - // TODO: The replica should return what information it knows about the current epoch and - // leader. return buildEmptyFetchResponse(errorOpt.get(), Optional.empty()); } @@ -1052,30 +1050,27 @@ private boolean handleFetchResponse( // The leader is asking us to fetch a snapshot if (partitionResponse.snapshotId().epoch() < 0) { - throw new KafkaException( - String.format( - "The leader sent a snapshot id with a valid end offset %s but with an invalid epoch %s", - partitionResponse.snapshotId().endOffset(), - partitionResponse.snapshotId().epoch() - ) + logger.error( + "The leader sent a snapshot id with a valid end offset {} but with an invalid epoch {}", + partitionResponse.snapshotId().endOffset(), + partitionResponse.snapshotId().epoch() ); - } - if (partitionResponse.snapshotId().endOffset() < 0) { - throw new KafkaException( - String.format( - "The leader sent a snapshot id with a valid epoch %s but with an invalid end offset %s", - partitionResponse.snapshotId().epoch(), - partitionResponse.snapshotId().endOffset() - ) + return false; + } else if (partitionResponse.snapshotId().endOffset() < 0) { + logger.error( + "The leader sent a snapshot id with a valid epoch {} but with an invalid end offset {}", + partitionResponse.snapshotId().epoch(), + partitionResponse.snapshotId().endOffset() + ); + return false; + } else { + OffsetAndEpoch snapshotId = new OffsetAndEpoch( + partitionResponse.snapshotId().endOffset(), + partitionResponse.snapshotId().epoch() ); - } - - OffsetAndEpoch snapshotId = new OffsetAndEpoch( - partitionResponse.snapshotId().endOffset(), - partitionResponse.snapshotId().epoch() - ); - state.setFetchingSnapshot(Optional.of(log.createSnapshot(snapshotId))); + state.setFetchingSnapshot(Optional.of(log.createSnapshot(snapshotId))); + } } else { Records records = (Records) partitionResponse.recordSet(); if (records.sizeInBytes() > 0) { @@ -1146,7 +1141,7 @@ private FetchSnapshotResponseData handleFetchSnapshotRequest( FetchSnapshotRequestData data = (FetchSnapshotRequestData) requestMetadata.data; if (data.topics().size() != 1 && data.topics().get(0).partitions().size() != 1) { - return FetchSnapshotResponse.withTopError(Errors.INVALID_REQUEST); + return FetchSnapshotResponse.withTopLevelError(Errors.INVALID_REQUEST); } Optional partitionSnapshotOpt = FetchSnapshotRequest @@ -1197,6 +1192,16 @@ private FetchSnapshotResponseData handleFetchSnapshotRequest( } try (RawSnapshotReader snapshot = snapshotOpt.get()) { + if (partitionSnapshot.position() < 0 || partitionSnapshot.position() >= snapshot.sizeInBytes()) { + return FetchSnapshotResponse.singleton( + log.topicPartition(), + responsePartitionSnapshot -> { + return addQuorumLeader(responsePartitionSnapshot) + .setErrorCode(Errors.POSITION_OUT_OF_RANGE.code()); + } + ); + } + int maxSnapshotSize; try { maxSnapshotSize = Math.toIntExact(snapshot.sizeInBytes()); @@ -1204,7 +1209,6 @@ private FetchSnapshotResponseData handleFetchSnapshotRequest( maxSnapshotSize = Integer.MAX_VALUE; } - // TODO: Make sure that we also limit based on the fetch max bytes configuration ByteBuffer buffer = ByteBuffer.allocate(Math.min(data.maxBytes(), maxSnapshotSize)); snapshot.read(buffer, partitionSnapshot.position()); buffer.flip(); @@ -1235,7 +1239,6 @@ private boolean handleFetchSnapshotResponse( FetchSnapshotResponseData data = (FetchSnapshotResponseData) responseMetadata.data; Errors topLevelError = Errors.forCode(data.errorCode()); if (topLevelError != Errors.NONE) { - // TODO: check what values this expression returns return handleTopLevelError(topLevelError, responseMetadata); } @@ -1259,7 +1262,6 @@ private boolean handleFetchSnapshotResponse( Optional handled = maybeHandleCommonResponse( error, responseLeaderId, responseEpoch, currentTimeMs); if (handled.isPresent()) { - // TODO: check what values this expression returns return handled.get(); } @@ -1272,6 +1274,7 @@ private boolean handleFetchSnapshotResponse( /* The leader deleted the snapshot before the follower could download it. Start over by * reseting the fetching snapshot state and sending another fetch request. */ + logger.trace("Leader doesn't know about snapshot id {}, returned error {} and snapshot id {}", state.fetchingSnapshot(), partitionSnapshot.errorCode(), partitionSnapshot.snapshotId()); state.setFetchingSnapshot(Optional.empty()); state.resetFetchTimeout(currentTimeMs); return true; @@ -1286,7 +1289,7 @@ private boolean handleFetchSnapshotResponse( if (state.fetchingSnapshot().isPresent()) { snapshot = state.fetchingSnapshot().get(); } else { - throw new IllegalStateException("Received unexpected fetch snapshot response: " + partitionSnapshot); + throw new IllegalStateException(String.format("Received unexpected fetch snapshot response: %s", partitionSnapshot)); } if (!snapshot.snapshotId().equals(snapshotId)) { @@ -1872,23 +1875,7 @@ private long pollFollowerAsVoter(FollowerState state, long currentTimeMs) throws transitionToCandidate(currentTimeMs); return 0L; } else { - long backoffMs; - if (state.fetchingSnapshot().isPresent()) { - RawSnapshotWriter snapshot = state.fetchingSnapshot().get(); - long snapshotSize = snapshot.sizeInBytes(); - - backoffMs = maybeSendRequest( - currentTimeMs, - state.leaderId(), - () -> buildFetchSnapshotRequest(snapshot.snapshotId(), snapshotSize) - ); - } else { - backoffMs = maybeSendRequest( - currentTimeMs, - state.leaderId(), - this::buildFetchRequest - ); - } + long backoffMs = maybeSendFetchOrFetchSnapshot(state, currentTimeMs); return Math.min(backoffMs, state.remainingFetchTimeMs(currentTimeMs)); } @@ -1910,28 +1897,28 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) thr } else if (connection.isBackingOff(currentTimeMs)) { backoffMs = maybeSendAnyVoterFetch(currentTimeMs); } else { - if (state.fetchingSnapshot().isPresent()) { - RawSnapshotWriter snapshot = state.fetchingSnapshot().get(); - long snapshotSize = snapshot.sizeInBytes(); - - backoffMs = maybeSendRequest( - currentTimeMs, - state.leaderId(), - () -> buildFetchSnapshotRequest(snapshot.snapshotId(), snapshotSize) - ); - } else { - backoffMs = maybeSendRequest( - currentTimeMs, - state.leaderId(), - this::buildFetchRequest - ); - } + backoffMs = maybeSendFetchOrFetchSnapshot(state, currentTimeMs); } return Math.min(backoffMs, state.remainingFetchTimeMs(currentTimeMs)); } } + private long maybeSendFetchOrFetchSnapshot(FollowerState state, long currentTimeMs) throws IOException { + final Supplier requestSupplier; + + if (state.fetchingSnapshot().isPresent()) { + RawSnapshotWriter snapshot = state.fetchingSnapshot().get(); + long snapshotSize = snapshot.sizeInBytes(); + + requestSupplier = () -> buildFetchSnapshotRequest(snapshot.snapshotId(), snapshotSize); + } else { + requestSupplier = this::buildFetchRequest; + } + + return maybeSendRequest(currentTimeMs, state.leaderId(), requestSupplier); + } + private long pollVoted(long currentTimeMs) throws IOException { VotedState state = quorum.votedStateOrThrow(); GracefulShutdown shutdown = this.shutdown.get(); diff --git a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java index e8c375ea2f252..86de6c0a04da5 100644 --- a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java +++ b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java @@ -596,6 +596,12 @@ public void testFetchSnapshotResponseToNotFollower() throws Exception { assertTrue(false); } + @Disabled + @Test + public void testFetchSnapshotResponseWithInvalidId() throws Exception { + assertTrue(false); + } + private static FetchSnapshotRequestData fetchSnapshotRequest( TopicPartition topicPartition, int epoch, From 2393cf75bb0f3854ae10d629c09207a5c0675640 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Tue, 22 Dec 2020 15:20:14 -0800 Subject: [PATCH 08/10] Add the remaining tests --- .../apache/kafka/raft/KafkaRaftClient.java | 11 +- .../raft/KafkaRaftClientSnapshotTest.java | 419 +++++++++++++++++- .../kafka/raft/RaftClientTestContext.java | 16 +- 3 files changed, 418 insertions(+), 28 deletions(-) diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index 5b29ef9178162..246ac1f337413 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -1055,14 +1055,14 @@ private boolean handleFetchResponse( partitionResponse.snapshotId().endOffset(), partitionResponse.snapshotId().epoch() ); - return false; + return true; } else if (partitionResponse.snapshotId().endOffset() < 0) { logger.error( "The leader sent a snapshot id with a valid epoch {} but with an invalid end offset {}", partitionResponse.snapshotId().epoch(), partitionResponse.snapshotId().endOffset() ); - return false; + return true; } else { OffsetAndEpoch snapshotId = new OffsetAndEpoch( partitionResponse.snapshotId().endOffset(), @@ -1274,7 +1274,12 @@ private boolean handleFetchSnapshotResponse( /* The leader deleted the snapshot before the follower could download it. Start over by * reseting the fetching snapshot state and sending another fetch request. */ - logger.trace("Leader doesn't know about snapshot id {}, returned error {} and snapshot id {}", state.fetchingSnapshot(), partitionSnapshot.errorCode(), partitionSnapshot.snapshotId()); + logger.trace( + "Leader doesn't know about snapshot id {}, returned error {} and snapshot id {}", + state.fetchingSnapshot(), + partitionSnapshot.errorCode(), + partitionSnapshot.snapshotId() + ); state.setFetchingSnapshot(Optional.empty()); state.resetFetchTimeout(currentTimeMs); return true; diff --git a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java index 86de6c0a04da5..ef2d2213fa888 100644 --- a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java +++ b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java @@ -38,7 +38,6 @@ import org.apache.kafka.snapshot.SnapshotWriter; import org.apache.kafka.snapshot.SnapshotWriterTest; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.Disabled; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -63,7 +62,7 @@ public void testMissingFetchSnapshotRequest() throws Exception { context.client.poll(); - FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); assertEquals(Errors.SNAPSHOT_NOT_FOUND, Errors.forCode(response.errorCode())); } @@ -88,7 +87,7 @@ public void testUnknownFetchSnapshotRequest() throws Exception { context.client.poll(); - FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(topicPartition).get(); + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(topicPartition).get(); assertEquals(Errors.UNKNOWN_TOPIC_OR_PARTITION, Errors.forCode(response.errorCode())); } @@ -120,7 +119,7 @@ public void testFetchSnapshotRequestAsLeader() throws Exception { context.client.poll(); - FetchSnapshotResponseData.PartitionSnapshot response = context + FetchSnapshotResponseData.PartitionSnapshot response = context .assertSentFetchSnapshotResponse(context.metadataPartition) .get(); @@ -235,22 +234,172 @@ public void testFetchSnapshotRequestAsFollower() throws IOException { context.client.poll(); - FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); assertEquals(Errors.NOT_LEADER_OR_FOLLOWER, Errors.forCode(response.errorCode())); assertEquals(epoch, response.currentLeader().leaderEpoch()); assertEquals(leaderId, response.currentLeader().leaderId()); } - @Disabled @Test - public void testFetchSnapshotRequestWithOlderEpoch() throws IOException { - assertTrue(false); + public void testFetchSnapshotRequestWithInvalidPosition() throws Exception { + int localId = 0; + Set voters = Utils.mkSet(localId, localId + 1); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(0, 0); + List records = Arrays.asList("foo", "bar"); + + RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); + + try (SnapshotWriter snapshot = context.client.createSnapshot(snapshotId)) { + snapshot.append(records); + snapshot.freeze(); + } + + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch, + snapshotId, + Integer.MAX_VALUE, + -1 + ) + ); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + assertEquals(Errors.POSITION_OUT_OF_RANGE, Errors.forCode(response.errorCode())); + assertEquals(epoch, response.currentLeader().leaderEpoch()); + assertEquals(localId, response.currentLeader().leaderId()); + + try (RawSnapshotReader snapshot = context.log.readSnapshot(snapshotId).get()) { + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch, + snapshotId, + Integer.MAX_VALUE, + snapshot.sizeInBytes() + ) + ); + + context.client.poll(); + + response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + assertEquals(Errors.POSITION_OUT_OF_RANGE, Errors.forCode(response.errorCode())); + assertEquals(epoch, response.currentLeader().leaderEpoch()); + assertEquals(localId, response.currentLeader().leaderId()); + } + } + + @Test + public void testFetchSnapshotRequestWithOlderEpoch() throws Exception { + int localId = 0; + Set voters = Utils.mkSet(localId, localId + 1); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(0, 0); + + RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); + + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch - 1, + snapshotId, + Integer.MAX_VALUE, + 0 + ) + ); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + assertEquals(Errors.FENCED_LEADER_EPOCH, Errors.forCode(response.errorCode())); + assertEquals(epoch, response.currentLeader().leaderEpoch()); + assertEquals(localId, response.currentLeader().leaderId()); } - @Disabled @Test - public void testFetchSnapshotRequestWithNewerEpoch() throws IOException { - assertTrue(false); + public void testFetchSnapshotRequestWithNewerEpoch() throws Exception { + int localId = 0; + Set voters = Utils.mkSet(localId, localId + 1); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(0, 0); + + RaftClientTestContext context = RaftClientTestContext.initializeAsLeader(localId, voters, epoch); + + context.deliverRequest( + fetchSnapshotRequest( + context.metadataPartition, + epoch + 1, + snapshotId, + Integer.MAX_VALUE, + 0 + ) + ); + + context.client.poll(); + + FetchSnapshotResponseData.PartitionSnapshot response = context.assertSentFetchSnapshotResponse(context.metadataPartition).get(); + assertEquals(Errors.UNKNOWN_LEADER_EPOCH, Errors.forCode(response.errorCode())); + assertEquals(epoch, response.currentLeader().leaderEpoch()); + assertEquals(localId, response.currentLeader().leaderId()); + } + + @Test + public void testFetchResponseWithInvalidSnapshotId() throws Exception { + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch invalidEpoch = new OffsetAndEpoch(100L, -1); + OffsetAndEpoch invalidEndOffset = new OffsetAndEpoch(-1L, 1); + int slept = 0; + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.time.sleep(1); + slept += 1; + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, invalidEpoch, 200L) + ); + + context.time.sleep(1); + slept += 1; + + context.pollUntilSend(); + fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, invalidEndOffset, 200L) + ); + + context.time.sleep(1); + slept += 1; + + context.pollUntilSend(); + fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + // Fetch timer is not reset; sleeping for remainder should transition to candidate + context.time.sleep(context.fetchTimeoutMs - slept); + + context.pollUntilSend(); + + context.assertSentVoteRequest(epoch + 1, 0, 0L); + context.assertVotedCandidate(epoch + 1, context.localId); } @Test @@ -583,23 +732,251 @@ public void testFetchSnapshotResponseFromNewerEpochLeader() throws Exception { context.assertFetchRequestData(fetchRequest, epoch + 1, 0L, 0); } - @Disabled @Test public void testFetchSnapshotResponseFromOlderEpoch() throws Exception { - assertTrue(false); - } + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); - @Disabled - @Test - public void testFetchSnapshotResponseToNotFollower() throws Exception { - assertTrue(false); + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + // Reply with unknown leader epoch + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + FetchSnapshotResponse.singleton( + context.metadataPartition, + responsePartitionSnapshot -> { + responsePartitionSnapshot + .currentLeader() + .setLeaderEpoch(epoch - 1) + .setLeaderId(leaderId + 1); + + return responsePartitionSnapshot + .setErrorCode(Errors.UNKNOWN_LEADER_EPOCH.code()); + } + ) + ); + + context.pollUntilSend(); + + // Follower should resend the fetch snapshot request + snapshotRequest = context.assertSentFetchSnapshotRequest(); + request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); } - @Disabled @Test public void testFetchSnapshotResponseWithInvalidId() throws Exception { - assertTrue(false); + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + // Reply with an invalid snapshot id endOffset + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + FetchSnapshotResponse.singleton( + context.metadataPartition, + responsePartitionSnapshot -> { + responsePartitionSnapshot + .currentLeader() + .setLeaderEpoch(epoch) + .setLeaderId(leaderId); + + responsePartitionSnapshot + .snapshotId() + .setEndOffset(-1) + .setEpoch(snapshotId.epoch); + + return responsePartitionSnapshot; + } + ) + ); + + context.pollUntilSend(); + + // Follower should send a fetch request + fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + + snapshotRequest = context.assertSentFetchSnapshotRequest(); + request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + // Reply with an invalid snapshot id epoch + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + FetchSnapshotResponse.singleton( + context.metadataPartition, + responsePartitionSnapshot -> { + responsePartitionSnapshot + .currentLeader() + .setLeaderEpoch(epoch) + .setLeaderId(leaderId); + + responsePartitionSnapshot + .snapshotId() + .setEndOffset(snapshotId.offset) + .setEpoch(-1); + + return responsePartitionSnapshot; + } + ) + ); + + context.pollUntilSend(); + + // Follower should send a fetch request + fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + } + + @Test + public void testFetchSnapshotResponseToNotFollower() throws Exception { + int localId = 0; + int leaderId = localId + 1; + Set voters = Utils.mkSet(localId, leaderId); + int epoch = 2; + OffsetAndEpoch snapshotId = new OffsetAndEpoch(100L, 1); + + RaftClientTestContext context = new RaftClientTestContext.Builder(localId, voters) + .withElectedLeader(epoch, leaderId) + .build(); + + context.pollUntilSend(); + RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); + context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); + + context.deliverResponse( + fetchRequest.correlationId, + fetchRequest.destinationId(), + snapshotFetchResponse(context.metadataPartition, epoch, leaderId, snapshotId, 200L) + ); + + context.pollUntilSend(); + + RaftRequest.Outbound snapshotRequest = context.assertSentFetchSnapshotRequest(); + FetchSnapshotRequestData.PartitionSnapshot request = assertFetchSnapshotRequest( + snapshotRequest, + context.metadataPartition, + localId, + Integer.MAX_VALUE + ).get(); + assertEquals(snapshotId.offset, request.snapshotId().endOffset()); + assertEquals(snapshotId.epoch, request.snapshotId().epoch()); + assertEquals(0, request.position()); + + // Sleeping for fetch timeout should transition to candidate + context.time.sleep(context.fetchTimeoutMs); + + context.pollUntilSend(); + + context.assertSentVoteRequest(epoch + 1, 0, 0L); + context.assertVotedCandidate(epoch + 1, context.localId); + + // Send the response late + context.deliverResponse( + snapshotRequest.correlationId, + snapshotRequest.destinationId(), + FetchSnapshotResponse.singleton( + context.metadataPartition, + responsePartitionSnapshot -> { + responsePartitionSnapshot + .currentLeader() + .setLeaderEpoch(epoch) + .setLeaderId(leaderId); + + responsePartitionSnapshot + .snapshotId() + .setEndOffset(snapshotId.offset) + .setEpoch(-1); + + return responsePartitionSnapshot; + } + ) + ); + + // Assert that the response is ignored and the replicas stays as a candidate + context.client.poll(); + context.assertVotedCandidate(epoch + 1, context.localId); } private static FetchSnapshotRequestData fetchSnapshotRequest( @@ -637,7 +1014,7 @@ private static FetchSnapshotResponseData fetchSnapshotResponse( ) { return FetchSnapshotResponse.singleton( topicPartition, - partitionSnapshot -> { + partitionSnapshot -> { partitionSnapshot.currentLeader() .setLeaderEpoch(leaderEpoch) .setLeaderId(leaderId); diff --git a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java index f5d52466d526b..d228fa4cb105a 100644 --- a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java +++ b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java @@ -85,12 +85,13 @@ public final class RaftClientTestContext { final TopicPartition metadataPartition = Builder.METADATA_PARTITION; final int electionBackoffMaxMs = Builder.ELECTION_BACKOFF_MAX_MS; - final int electionTimeoutMs = Builder.DEFAULT_ELECTION_TIMEOUT_MS; final int electionFetchMaxWaitMs = Builder.FETCH_MAX_WAIT_MS; final int fetchTimeoutMs = Builder.FETCH_TIMEOUT_MS; - final int requestTimeoutMs = Builder.DEFAULT_REQUEST_TIMEOUT_MS; final int retryBackoffMs = Builder.RETRY_BACKOFF_MS; + final int electionTimeoutMs; + final int requestTimeoutMs; + private final QuorumStateStore quorumStateStore; final int localId; public final KafkaRaftClient client; @@ -223,7 +224,9 @@ public RaftClientTestContext build() throws IOException { quorumStateStore, voters, metrics, - listener + listener, + electionTimeoutMs, + requestTimeoutMs ); } } @@ -237,7 +240,9 @@ private RaftClientTestContext( QuorumStateStore quorumStateStore, Set voters, Metrics metrics, - MockListener listener + MockListener listener, + int electionTimeoutMs, + int requestTimeoutMs ) { this.localId = localId; this.client = client; @@ -248,6 +253,9 @@ private RaftClientTestContext( this.voters = voters; this.metrics = metrics; this.listener = listener; + + this.electionTimeoutMs = electionTimeoutMs; + this.requestTimeoutMs = requestTimeoutMs; } MemoryRecords buildBatch( From cad8284a4333fffb5b15c09b0d71a0a38a8cd7c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Tue, 22 Dec 2020 17:30:48 -0800 Subject: [PATCH 09/10] Invalid snapshot should back off instead of reporting success --- .../kafka/tools/TestRaftRequestHandler.scala | 4 ++-- .../apache/kafka/raft/KafkaRaftClient.java | 8 +++++--- .../raft/KafkaRaftClientSnapshotTest.java | 19 ++++++++++++------- .../kafka/raft/RaftClientTestContext.java | 2 +- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala b/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala index b0878207f76ee..298baf875b788 100644 --- a/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala +++ b/core/src/main/scala/kafka/tools/TestRaftRequestHandler.scala @@ -22,10 +22,10 @@ import kafka.network.RequestConvertToJson import kafka.server.ApiRequestHandler import kafka.utils.Logging import org.apache.kafka.common.internals.FatalExitError -import org.apache.kafka.common.message.{BeginQuorumEpochResponseData, EndQuorumEpochResponseData, FetchResponseData, VoteResponseData} +import org.apache.kafka.common.message.{BeginQuorumEpochResponseData, EndQuorumEpochResponseData, FetchResponseData, FetchSnapshotResponseData, VoteResponseData} import org.apache.kafka.common.protocol.{ApiKeys, ApiMessage, Errors} import org.apache.kafka.common.record.BaseRecords -import org.apache.kafka.common.requests.{AbstractRequest, AbstractResponse, BeginQuorumEpochResponse, EndQuorumEpochResponse, FetchResponse, VoteResponse} +import org.apache.kafka.common.requests.{AbstractRequest, AbstractResponse, BeginQuorumEpochResponse, EndQuorumEpochResponse, FetchResponse, FetchSnapshotResponse, VoteResponse} import org.apache.kafka.common.utils.Time import org.apache.kafka.raft.{KafkaRaftClient, RaftRequest} diff --git a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java index d7c8f683df07d..241e08cf7d778 100644 --- a/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java +++ b/raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java @@ -1071,14 +1071,14 @@ private boolean handleFetchResponse( partitionResponse.snapshotId().endOffset(), partitionResponse.snapshotId().epoch() ); - return true; + return false; } else if (partitionResponse.snapshotId().endOffset() < 0) { logger.error( "The leader sent a snapshot id with a valid epoch {} but with an invalid end offset {}", partitionResponse.snapshotId().epoch(), partitionResponse.snapshotId().endOffset() ); - return true; + return false; } else { OffsetAndEpoch snapshotId = new OffsetAndEpoch( partitionResponse.snapshotId().endOffset(), @@ -1609,7 +1609,9 @@ private long maybeSendRequest( ConnectionState connection = requestManager.getOrCreate(destinationId); if (connection.isBackingOff(currentTimeMs)) { - return connection.remainingBackoffMs(currentTimeMs); + long remainingBackoffMs = connection.remainingBackoffMs(currentTimeMs); + logger.debug("Connection for {} is backing off for {} ms", destinationId, remainingBackoffMs); + return remainingBackoffMs; } if (connection.isReady(currentTimeMs)) { diff --git a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java index 96d8c71371c47..61fb5b9c16eb4 100644 --- a/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java +++ b/raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java @@ -360,9 +360,6 @@ public void testFetchResponseWithInvalidSnapshotId() throws Exception { .withElectedLeader(epoch, leaderId) .build(); - context.time.sleep(1); - slept += 1; - context.pollUntilRequest(); RaftRequest.Outbound fetchRequest = context.assertSentFetchRequest(); context.assertFetchRequestData(fetchRequest, epoch, 0L, 0); @@ -373,8 +370,12 @@ public void testFetchResponseWithInvalidSnapshotId() throws Exception { snapshotFetchResponse(context.metadataPartition, epoch, leaderId, invalidEpoch, 200L) ); - context.time.sleep(1); - slept += 1; + // Handle the invalid response + context.client.poll(); + + // Expect another fetch request after backoff has expired + context.time.sleep(context.retryBackoffMs); + slept += context.retryBackoffMs; context.pollUntilRequest(); fetchRequest = context.assertSentFetchRequest(); @@ -386,8 +387,12 @@ public void testFetchResponseWithInvalidSnapshotId() throws Exception { snapshotFetchResponse(context.metadataPartition, epoch, leaderId, invalidEndOffset, 200L) ); - context.time.sleep(1); - slept += 1; + // Handle the invalid response + context.client.poll(); + + // Expect another fetch request after backoff has expired + context.time.sleep(context.retryBackoffMs); + slept += context.retryBackoffMs; context.pollUntilRequest(); fetchRequest = context.assertSentFetchRequest(); diff --git a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java index cf3b35eb27455..fc342c1db02de 100644 --- a/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java +++ b/raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java @@ -374,7 +374,7 @@ void pollUntil(TestCondition condition) throws InterruptedException { TestUtils.waitForCondition(() -> { client.poll(); return condition.conditionMet(); - }, 500000000, "Condition failed to be satisfied before timeout"); + }, 5000, "Condition failed to be satisfied before timeout"); } void pollUntilResponse() throws InterruptedException { From 22133d1166394ea4ffbf111aeb8eb063c2182cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Armando=20Garc=C3=ADa=20Sancio?= Date: Wed, 23 Dec 2020 10:09:54 -0800 Subject: [PATCH 10/10] Add missing parsing code --- .../common/requests/AbstractRequest.java | 2 ++ .../common/requests/AbstractResponse.java | 2 ++ .../common/requests/FetchSnapshotRequest.java | 27 ++++++++++++++++++- .../requests/FetchSnapshotResponse.java | 6 +++++ .../kafka/network/RequestConvertToJson.scala | 2 ++ .../kafka/raft/KafkaNetworkChannel.scala | 3 +++ 6 files changed, 41 insertions(+), 1 deletion(-) diff --git a/clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java b/clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java index ca58e82912adc..6dbd3d6468020 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java @@ -272,6 +272,8 @@ private static AbstractRequest doParseRequest(ApiKeys apiKey, short apiVersion, return UpdateFeaturesRequest.parse(buffer, apiVersion); case ENVELOPE: return EnvelopeRequest.parse(buffer, apiVersion); + case FETCH_SNAPSHOT: + return FetchSnapshotRequest.parse(buffer, apiVersion); default: throw new AssertionError(String.format("ApiKey %s is not currently handled in `parseRequest`, the " + "code should be updated to do so.", apiKey)); diff --git a/clients/src/main/java/org/apache/kafka/common/requests/AbstractResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/AbstractResponse.java index 26eda3387e023..f41b68fb08c84 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/AbstractResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/AbstractResponse.java @@ -229,6 +229,8 @@ public static AbstractResponse parseResponse(ApiKeys apiKey, ByteBuffer response return UpdateFeaturesResponse.parse(responseBuffer, version); case ENVELOPE: return EnvelopeResponse.parse(responseBuffer, version); + case FETCH_SNAPSHOT: + return FetchSnapshotResponse.parse(responseBuffer, version); default: throw new AssertionError(String.format("ApiKey %s is not currently handled in `parseResponse`, the " + "code should be updated to do so.", apiKey)); diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java index 76d14a72f81be..30952ac77e80e 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotRequest.java @@ -16,6 +16,7 @@ */ package org.apache.kafka.common.requests; +import java.nio.ByteBuffer; import java.util.Collections; import java.util.Optional; import java.util.function.UnaryOperator; @@ -24,12 +25,13 @@ import org.apache.kafka.common.message.FetchSnapshotResponseData; import org.apache.kafka.common.protocol.ApiKeys; import org.apache.kafka.common.protocol.ApiMessage; +import org.apache.kafka.common.protocol.ByteBufferAccessor; import org.apache.kafka.common.protocol.Errors; final public class FetchSnapshotRequest extends AbstractRequest { public final FetchSnapshotRequestData data; - public FetchSnapshotRequest(FetchSnapshotRequestData data) { + public FetchSnapshotRequest(FetchSnapshotRequestData data, short version) { super(ApiKeys.FETCH_SNAPSHOT, (short) (FetchSnapshotRequestData.SCHEMAS.length - 1)); this.data = data; } @@ -94,4 +96,27 @@ public static Optional forTopicParti .filter(partition -> partition.partition() == topicPartition.partition()) .findAny(); } + + public static FetchSnapshotRequest parse(ByteBuffer buffer, short version) { + return new FetchSnapshotRequest(new FetchSnapshotRequestData(new ByteBufferAccessor(buffer), version), version); + } + + public static class Builder extends AbstractRequest.Builder { + private final FetchSnapshotRequestData data; + + public Builder(FetchSnapshotRequestData data) { + super(ApiKeys.FETCH_SNAPSHOT); + this.data = data; + } + + @Override + public FetchSnapshotRequest build(short version) { + return new FetchSnapshotRequest(data, version); + } + + @Override + public String toString() { + return data.toString(); + } + } } diff --git a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java index 9f3eedcfc8223..22e1f81bd7a2d 100644 --- a/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java +++ b/clients/src/main/java/org/apache/kafka/common/requests/FetchSnapshotResponse.java @@ -16,6 +16,7 @@ */ package org.apache.kafka.common.requests; +import java.nio.ByteBuffer; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -25,6 +26,7 @@ import org.apache.kafka.common.message.FetchSnapshotResponseData; import org.apache.kafka.common.protocol.ApiKeys; import org.apache.kafka.common.protocol.ApiMessage; +import org.apache.kafka.common.protocol.ByteBufferAccessor; import org.apache.kafka.common.protocol.Errors; final public class FetchSnapshotResponse extends AbstractResponse { @@ -121,4 +123,8 @@ public static Optional forTopicPart .filter(parition -> parition.index() == topicPartition.partition()) .findAny(); } + + public static FetchSnapshotResponse parse(ByteBuffer buffer, short version) { + return new FetchSnapshotResponse(new FetchSnapshotResponseData(new ByteBufferAccessor(buffer), version)); + } } diff --git a/core/src/main/scala/kafka/network/RequestConvertToJson.scala b/core/src/main/scala/kafka/network/RequestConvertToJson.scala index f610df9846b51..db3a11b68b592 100644 --- a/core/src/main/scala/kafka/network/RequestConvertToJson.scala +++ b/core/src/main/scala/kafka/network/RequestConvertToJson.scala @@ -86,6 +86,7 @@ object RequestConvertToJson { case req: UpdateMetadataRequest => UpdateMetadataRequestDataJsonConverter.write(req.data, request.version) case req: VoteRequest => VoteRequestDataJsonConverter.write(req.data, request.version) case req: WriteTxnMarkersRequest => WriteTxnMarkersRequestDataJsonConverter.write(req.data, request.version) + case req: FetchSnapshotRequest => FetchSnapshotRequestDataJsonConverter.write(req.data, request.version) case _ => throw new IllegalStateException(s"ApiKey ${request.apiKey} is not currently handled in `request`, the " + "code should be updated to do so."); } @@ -152,6 +153,7 @@ object RequestConvertToJson { case res: UpdateMetadataResponse => UpdateMetadataResponseDataJsonConverter.write(res.data, version) case res: WriteTxnMarkersResponse => WriteTxnMarkersResponseDataJsonConverter.write(res.data, version) case res: VoteResponse => VoteResponseDataJsonConverter.write(res.data, version) + case res: FetchSnapshotResponse => FetchSnapshotResponseDataJsonConverter.write(res.data, version) case _ => throw new IllegalStateException(s"ApiKey ${response.apiKey} is not currently handled in `response`, the " + "code should be updated to do so."); } diff --git a/core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala b/core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala index 435167208f70a..8c121b06cf274 100644 --- a/core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala +++ b/core/src/main/scala/kafka/raft/KafkaNetworkChannel.scala @@ -46,7 +46,10 @@ object KafkaNetworkChannel { // Since we already have the request, we go through a simplified builder new AbstractRequest.Builder[FetchRequest](ApiKeys.FETCH) { override def build(version: Short): FetchRequest = new FetchRequest(fetchRequest, version) + override def toString(): String = fetchRequest.toString } + case fetchSnapshotRequest: FetchSnapshotRequestData => + new FetchSnapshotRequest.Builder(fetchSnapshotRequest) case _ => throw new IllegalArgumentException(s"Unexpected type for requestData: $requestData") }