hachikuji commented on a change in pull request #8979: URL: https://github.com/apache/kafka/pull/8979#discussion_r455413527
########## File path: clients/src/main/java/org/apache/kafka/common/errors/NotLeaderOrFollowerException.java ########## @@ -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 { Review comment: One nitpick on the naming. It seems a little strange that followers might return `NOT_LEADER_OR_FOLLOWER` for 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. ########## File path: clients/src/main/java/org/apache/kafka/common/protocol/Errors.java ########## @@ -139,8 +139,8 @@ 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.", Review comment: This would also be a good place to be clearer about the interpretation of this error. Maybe something like this? > For requests intended only for the leader, this error indicates that the broker is not the current leader. For requests intended for any replica, this error indicates that the broker is not a replica of the topic partition. ########## File path: clients/src/main/java/org/apache/kafka/common/protocol/Errors.java ########## @@ -139,8 +139,8 @@ 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.", + NotLeaderOrFollowerException::new), REQUEST_TIMED_OUT(7, "The request timed out.", TimeoutException::new), BROKER_NOT_AVAILABLE(8, "The broker is not available.", Review comment: Is it worth adding a note about the use of REPLICA_NOT_AVAILABLE below? I think we are effectively deprecating its usage in this PR. ########## File path: core/src/main/scala/kafka/server/ReplicaManager.scala ########## @@ -712,10 +708,13 @@ class ReplicaManager(val config: KafkaConfig, } catch { case e@(_: InvalidTopicException | _: LogDirNotFoundException | - _: ReplicaNotAvailableException | _: KafkaStorageException) => - warn("Unable to alter log dirs for %s".format(topicPartition), e) + warn(s"Unable to alter log dirs for $topicPartition", e) (topicPartition, Errors.forException(e)) + case e: NotLeaderOrFollowerException => + warn(s"Unable to alter log dirs for $topicPartition", e) + // Retaining REPLICA_NOT_AVAILABLE exception for ALTER_REPLICA_LOG_DIRS for older versions for compatibility + (topicPartition, if (config.interBrokerProtocolVersion >= KAFKA_2_7_IV0) Errors.NOT_LEADER_OR_FOLLOWER else Errors.REPLICA_NOT_AVAILABLE) Review comment: Hmm.. Wouldn't it make more sense to tie this to a version bump to this protocol rather than the IBP? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
