You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/08/15 22:15:18 UTC
[GitHub] [kafka] hachikuji opened a new pull request, #12517: KAFKA-13940; Return NOT_LEADER_OR_FOLLOWER if DescribeQuorum sent to non-leader
hachikuji opened a new pull request, #12517:
URL: https://github.com/apache/kafka/pull/12517
Currently the server will return `INVALID_REQUEST` if a `DescribeQuorum` request is sent to a node that is not the current leader. In addition to being inconsistent with all of the other leader APIs, this error is treated as fatal by both the forwarding manager and the admin client. Instead, we should return `NOT_LEADER_OR_FOLLOWER` as we do with the other APIs. This error is retriable and we rely on the admin client to retry it after seeing this error.
Note that we could also check for this error in `ForwardingManager` and retry the request from that context, but the current forwarding logic is only setup to handle retries for requests sent to the controller, which is logically distinct from the raft leader. The `DescribeQuorum` is a bit of an oddball from an API perspective because it is a multi-partition request, but it is always forwarded to the controller since there is only one raft partition. So to handle this case specially, we would have to unwrap the envelope and check the error for the __cluster_metadata partition. It seemed simpler to rely on the admin client to retry.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #12517: KAFKA-13940; Return NOT_LEADER_OR_FOLLOWER if DescribeQuorum sent to non-leader
Posted by GitBox <gi...@apache.org>.
hachikuji merged PR #12517:
URL: https://github.com/apache/kafka/pull/12517
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a diff in pull request #12517: KAFKA-13940; Return NOT_LEADER_OR_FOLLOWER if DescribeQuorum sent to non-leader
Posted by GitBox <gi...@apache.org>.
jsancio commented on code in PR #12517:
URL: https://github.com/apache/kafka/pull/12517#discussion_r946877557
##########
clients/src/main/java/org/apache/kafka/common/requests/DescribeQuorumResponse.java:
##########
@@ -72,6 +72,18 @@ public int throttleTimeMs() {
return DEFAULT_THROTTLE_TIME;
}
+ public static DescribeQuorumResponseData singletonErrorResponse(
+ TopicPartition topicPartition,
+ Errors error
+ ) {
+ return new DescribeQuorumResponseData()
+ .setTopics(Collections.singletonList(new DescribeQuorumResponseData.TopicData()
+ .setTopicName(topicPartition.topic())
+ .setPartitions(Collections.singletonList(new DescribeQuorumResponseData.PartitionData()
+ .setErrorCode(error.code())))));
Review Comment:
I agree, we should use the partition index from `topicPartition`.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a diff in pull request #12517: KAFKA-13940; Return NOT_LEADER_OR_FOLLOWER if DescribeQuorum sent to non-leader
Posted by GitBox <gi...@apache.org>.
dengziming commented on code in PR #12517:
URL: https://github.com/apache/kafka/pull/12517#discussion_r947668414
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -1171,7 +1171,10 @@ private DescribeQuorumResponseData handleDescribeQuorumRequest(
}
if (!quorum.isLeader()) {
- return DescribeQuorumRequest.getTopLevelErrorResponse(Errors.INVALID_REQUEST);
+ return DescribeQuorumResponse.singletonErrorResponse(
Review Comment:
We have documentation comments for `handleFetchRequest` and other `handleXXXRequest` methods, how about adding documentation comments for `handleDescribeQuorumRequest`.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a diff in pull request #12517: KAFKA-13940; Return NOT_LEADER_OR_FOLLOWER if DescribeQuorum sent to non-leader
Posted by GitBox <gi...@apache.org>.
hachikuji commented on code in PR #12517:
URL: https://github.com/apache/kafka/pull/12517#discussion_r946937277
##########
clients/src/main/java/org/apache/kafka/common/requests/DescribeQuorumResponse.java:
##########
@@ -72,6 +72,18 @@ public int throttleTimeMs() {
return DEFAULT_THROTTLE_TIME;
}
+ public static DescribeQuorumResponseData singletonErrorResponse(
+ TopicPartition topicPartition,
+ Errors error
+ ) {
+ return new DescribeQuorumResponseData()
+ .setTopics(Collections.singletonList(new DescribeQuorumResponseData.TopicData()
+ .setTopicName(topicPartition.topic())
+ .setPartitions(Collections.singletonList(new DescribeQuorumResponseData.PartitionData()
+ .setErrorCode(error.code())))));
Review Comment:
Good catch. This was missing in `singletonResponse` as well.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a diff in pull request #12517: KAFKA-13940; Return NOT_LEADER_OR_FOLLOWER if DescribeQuorum sent to non-leader
Posted by GitBox <gi...@apache.org>.
hachikuji commented on code in PR #12517:
URL: https://github.com/apache/kafka/pull/12517#discussion_r948223003
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -1171,7 +1171,10 @@ private DescribeQuorumResponseData handleDescribeQuorumRequest(
}
if (!quorum.isLeader()) {
- return DescribeQuorumRequest.getTopLevelErrorResponse(Errors.INVALID_REQUEST);
+ return DescribeQuorumResponse.singletonErrorResponse(
Review Comment:
Hmm, this part at least seems self-explanatory. Do you see something else that deserves more explanation? Would you mind submitting a follow-up PR if you do? I am happy to review.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #12517: KAFKA-13940; Return NOT_LEADER_OR_FOLLOWER if DescribeQuorum sent to non-leader
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12517:
URL: https://github.com/apache/kafka/pull/12517#discussion_r946751960
##########
clients/src/main/java/org/apache/kafka/common/requests/DescribeQuorumResponse.java:
##########
@@ -72,6 +72,18 @@ public int throttleTimeMs() {
return DEFAULT_THROTTLE_TIME;
}
+ public static DescribeQuorumResponseData singletonErrorResponse(
+ TopicPartition topicPartition,
+ Errors error
+ ) {
+ return new DescribeQuorumResponseData()
+ .setTopics(Collections.singletonList(new DescribeQuorumResponseData.TopicData()
+ .setTopicName(topicPartition.topic())
+ .setPartitions(Collections.singletonList(new DescribeQuorumResponseData.PartitionData()
+ .setErrorCode(error.code())))));
Review Comment:
Should we also set the partition index based on `topicPartition` to be complete? It is zero by default so it works here but that could be a nasty bug if the helper is reused in another context.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org