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 because
true when the voter is successfully added to the voter set.
##########
raft/src/main/java/org/apache/kafka/raft/internals/KRaftControlRecordStateMachine.java:
##########
@@ -302,6 +310,7 @@ private void handleBatch(Batch<?> batch, OptionalLong
overrideOffset) {
case KRAFT_VOTERS:
VoterSet voters = VoterSet.fromVotersRecord((VotersRecord)
record.message());
kafkaRaftMetrics.updateNumVoters(voters.size());
+ seenReplicaKey.addAll(voters.voterKeys());
Review Comment:
Kafka should not have data structures that only grow in size. E.g. there is
no remove operation against `seenReplicaKey`. This means that the size of this
data structure is unbounded and will continue to grow even if the voter set is
as small as 3 to 5 voters.
##########
raft/src/main/java/org/apache/kafka/raft/internals/AddVoterHandler.java:
##########
@@ -156,6 +156,23 @@ public CompletableFuture<AddRaftVoterResponseData>
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 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]
