Re: [PR] MINOR: Document kRaft voter auto join will add a removed voter immediately issue [kafka]
jsancio commented on code in PR #21025:
URL: https://github.com/apache/kafka/pull/21025#discussion_r2582183275
##
raft/src/main/java/org/apache/kafka/raft/internals/AddVoterHandler.java:
##
@@ -156,6 +156,23 @@ public CompletableFuture
handleAddVoterRequest(
);
}
+
+if (!ackWhenCommitted && partitionState.hasSeenReplicaKey(voterKey)) {
+// ackWhenCommitted is used to distinguish the request from admin
+// or broker. If it comes from broker and had been in cluster, we
should
+// reject it.
Review Comment:
This is a peculiar association. Why should "ack when committed" requests
fail if the voter key is in the log?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] MINOR: Document kRaft voter auto join will add a removed voter immediately issue [kafka]
jsancio commented on code in PR #21025:
URL: https://github.com/apache/kafka/pull/21025#discussion_r2582197596
##
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##
@@ -2335,9 +2337,12 @@ private boolean handleAddVoterResponse(
/* These error codes indicate the replica was successfully added or
the leader is unable to
* process the request. In either case, reset the update voter set
timer to back off.
*/
-if (error == Errors.NONE || error == Errors.REQUEST_TIMED_OUT ||
-error == Errors.DUPLICATE_VOTER) {
+if (error == Errors.NONE || error == Errors.REQUEST_TIMED_OUT) {
+
quorum.followerStateOrThrow().resetUpdateVoterSetPeriod(currentTimeMs);
+return true;
+} else if (error == Errors.DUPLICATE_VOTER) {
quorum.followerStateOrThrow().resetUpdateVoterSetPeriod(currentTimeMs);
+hasJoined = true;
Review Comment:
This doesn't seem correct. You want to have the has joined boolean as true
when the voter is successfully added to the voter set.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] MINOR: Document kRaft voter auto join will add a removed voter immediately issue [kafka]
showuon commented on code in PR #21025: URL: https://github.com/apache/kafka/pull/21025#discussion_r2583521619 ## docs/upgrade.html: ## @@ -176,7 +176,8 @@ Notable changes in 4 The controller.quorum.auto.join.enable has been added to QuorumConfig, enabling KRaft controllers to automatically join the cluster's voter set, -and defaults to false. +and defaults to false. There is an issue which is KRaft voter auto join will add a removed voter immediately, +please see https://issues.apache.org/jira/browse/KAFKA-19850";>KAFKA-19850 Review Comment: I think no. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Document kRaft voter auto join will add a removed voter immediately issue [kafka]
TaiJuWu commented on code in PR #21025: URL: https://github.com/apache/kafka/pull/21025#discussion_r2583515850 ## docs/upgrade.html: ## @@ -176,7 +176,8 @@ Notable changes in 4 The controller.quorum.auto.join.enable has been added to QuorumConfig, enabling KRaft controllers to automatically join the cluster's voter set, -and defaults to false. +and defaults to false. There is an issue which is KRaft voter auto join will add a removed voter immediately, +please see https://issues.apache.org/jira/browse/KAFKA-19850";>KAFKA-19850 Review Comment: Should we mention Jira ticket or we also need to avoid it? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Document kRaft voter auto join will add a removed voter immediately issue [kafka]
showuon commented on code in PR #21025: URL: https://github.com/apache/kafka/pull/21025#discussion_r2583509801 ## docs/upgrade.html: ## @@ -176,7 +176,8 @@ Notable changes in 4 The controller.quorum.auto.join.enable has been added to QuorumConfig, enabling KRaft controllers to automatically join the cluster's voter set, -and defaults to false. +and defaults to false. There is an issue which is KRaft voter auto join will add a removed voter immediately, +please see https://issues.apache.org/jira/browse/KAFKA-19850";>KAFKA-19850 Review Comment: I would avoid to call it as an `issue` since we agree it works as expected. Maybe we can update it as: `If the configuration is set to true the controller must be shutdown before removing the controller from the voter set to avoid the removed controller to automatically join again.` WDYT? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] MINOR: Document kRaft voter auto join will add a removed voter immediately issue [kafka]
TaiJuWu commented on PR #21025: URL: https://github.com/apache/kafka/pull/21025#issuecomment-3604879943 @showuon @jsancio @kevin-wu24 , I updated this PR to document this issue only, PTAL. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
