-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-10223; Use NOT_LEADER_OR_FOLLOWER instead of non-retriable REPLICA_NOT_AVAILABLE for consumers #8979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-10223; Use NOT_LEADER_OR_FOLLOWER instead of non-retriable REPLICA_NOT_AVAILABLE for consumers #8979
Changes from 2 commits
116b7b0
8bcda5e
eedd6e2
991b61d
65d4f62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * 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; | ||
|
|
||
| /** | ||
| * This server is not the leader or follower for the given partition. | ||
| * This could a transient exception during reassignments. | ||
| */ | ||
| @SuppressWarnings("deprecation") | ||
| public class NotLeaderOrFollowerException extends NotLeaderForPartitionException { | ||
|
|
||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| public NotLeaderOrFollowerException() { | ||
| super(); | ||
| } | ||
|
|
||
| public NotLeaderOrFollowerException(String message) { | ||
| super(message); | ||
| } | ||
|
|
||
| public NotLeaderOrFollowerException(Throwable cause) { | ||
| super(cause); | ||
| } | ||
|
|
||
| public NotLeaderOrFollowerException(String message, Throwable cause) { | ||
| super(message, cause); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,7 +72,7 @@ | |
| import org.apache.kafka.common.errors.NotCoordinatorException; | ||
| import org.apache.kafka.common.errors.NotEnoughReplicasAfterAppendException; | ||
| import org.apache.kafka.common.errors.NotEnoughReplicasException; | ||
| import org.apache.kafka.common.errors.NotLeaderForPartitionException; | ||
| import org.apache.kafka.common.errors.NotLeaderOrFollowerException; | ||
| import org.apache.kafka.common.errors.OffsetMetadataTooLarge; | ||
| import org.apache.kafka.common.errors.OffsetNotAvailableException; | ||
| import org.apache.kafka.common.errors.OffsetOutOfRangeException; | ||
|
|
@@ -139,8 +139,8 @@ public enum Errors { | |
| InvalidFetchSizeException::new), | ||
| LEADER_NOT_AVAILABLE(5, "There is no leader for this topic-partition as we are in the middle of a leadership election.", | ||
| LeaderNotAvailableException::new), | ||
| NOT_LEADER_FOR_PARTITION(6, "This server is not the leader for that topic-partition.", | ||
| NotLeaderForPartitionException::new), | ||
| NOT_LEADER_OR_FOLLOWER(6, "This server is not the leader or follower for that topic-partition.", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would also be a good place to be clearer about the interpretation of this error. Maybe something like this?
|
||
| NotLeaderOrFollowerException::new), | ||
| REQUEST_TIMED_OUT(7, "The request timed out.", | ||
| TimeoutException::new), | ||
| BROKER_NOT_AVAILABLE(8, "The broker is not available.", | ||
|
hachikuji marked this conversation as resolved.
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,8 +58,8 @@ | |
| * | ||
| * - {@link Errors#OFFSET_OUT_OF_RANGE} If the fetch offset is out of range for a requested partition | ||
| * - {@link Errors#TOPIC_AUTHORIZATION_FAILED} If the user does not have READ access to a requested topic | ||
| * - {@link Errors#REPLICA_NOT_AVAILABLE} If the request is received by a broker which is not a replica | ||
| * - {@link Errors#NOT_LEADER_FOR_PARTITION} If the broker is not a leader and either the provided leader epoch | ||
| * - {@link Errors#REPLICA_NOT_AVAILABLE} If the request is received by a broker with version 2.4 to 2.6 which is not a replica | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the note about the range from 2.4 to 2.6 correct? I think this error has always been possible in the case of a reassignment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to say < 2.6. |
||
| * - {@link Errors#NOT_LEADER_OR_FOLLOWER} If the broker is not a leader or follower and either the provided leader epoch | ||
| * matches the known leader epoch on the broker or is empty | ||
| * - {@link Errors#FENCED_LEADER_EPOCH} If the epoch is lower than the broker's epoch | ||
| * - {@link Errors#UNKNOWN_LEADER_EPOCH} If the epoch is larger than the broker's epoch | ||
|
|
@@ -190,7 +190,7 @@ public class FetchResponse<T extends BaseRecords> extends AbstractResponse { | |
| new Field(RESPONSES_KEY_NAME, new ArrayOf(FETCH_RESPONSE_TOPIC_V5))); | ||
|
|
||
| // V6 bumped up to indicate that the client supports KafkaStorageException. The KafkaStorageException will | ||
| // be translated to NotLeaderForPartitionException in the response if version <= 5 | ||
| // be translated to NotLeaderOrFollowerException in the response if version <= 5 | ||
| private static final Schema FETCH_RESPONSE_V6 = FETCH_RESPONSE_V5; | ||
|
|
||
| // V7 added incremental fetch responses and a top-level error code. | ||
|
|
@@ -549,9 +549,9 @@ private static <T extends BaseRecords> Struct toStruct(short version, int thrott | |
| // If consumer sends FetchRequest V5 or earlier, the client library is not guaranteed to recognize the error code | ||
| // for KafkaStorageException. In this case the client library will translate KafkaStorageException to | ||
| // UnknownServerException which is not retriable. We can ensure that consumer will update metadata and retry | ||
| // by converting the KafkaStorageException to NotLeaderForPartitionException in the response if FetchRequest version <= 5 | ||
| // by converting the KafkaStorageException to NotLeaderOrFollowerException in the response if FetchRequest version <= 5 | ||
| if (errorCode == Errors.KAFKA_STORAGE_ERROR.code() && version <= 5) | ||
| errorCode = Errors.NOT_LEADER_FOR_PARTITION.code(); | ||
| errorCode = Errors.NOT_LEADER_OR_FOLLOWER.code(); | ||
| Struct partitionData = topicData.instance(PARTITIONS_KEY_NAME); | ||
| Struct partitionDataHeader = partitionData.instance(PARTITION_HEADER_KEY_NAME); | ||
| partitionDataHeader.set(PARTITION_ID, partitionEntry.getKey()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nitpick on the naming. It seems a little strange that followers might return
NOT_LEADER_OR_FOLLOWERfor requests which are intended for the leader. I think my suggestion to frame the name around the broker not being the intended target of the request was a little more accurate, though I admit the name was clumsier. Perhaps this class would be a good place to document the semantics? Maybe worth mentioning the Produce/Fetch behavior explicitly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated javadoc.