KAFKA-20865: Do not unregister KRaft voter set members by kevin-wu24 · Pull Request #23152 · apache/kafka

kevin-wu24 · 2026-08-13T14:59:05Z

What changed

Follow up PR to c274a73, which implements controller unregistration. This PR adds a check so that KRaft voter set members cannot be unregistered, which can limit the severity of weird edge cases around races between unregistration requests and feature upgrades. Voters who are unregistered and then re-register with a cluster who completed an unsupported feature upgrade thanks to the unregistration would experience unavailability, which affects quorum health.

The KRaft voter set is internal to the /raft module, so this PR adds the RaftClientVotersSupplier alongside some new public RaftClient API to expose the latest voter set. Only the latest is necessary, because with kraft.version=1 enabled, the controller being unregistered depends only on the uncommitted voter set when it is present, described below.

When dynamic quorum is enabled, the unregistration voter check only needs to check the uncommitted voter set, if one exists. This is because the unregistration record will be written, and thus committed, after the uncommitted VotersRecord if one exists. The logic is as follows for unregistering controller X, with the latest CVS (committed voter set) and UVS (uncommitted voter set):

CVS.contains(X) && UVS.contains(X): cannot unregister, because X is definitely a voter
CVS.contains(X) && !UVS.contains(X): can unregister, because the offset of X's unregistration's record will come after its removal from the voter set. X's unregistration cannot be committed without the new voter set also being committed first.
!CVS.contains(X) && UVS.contains(X): cannot unregister, because committing both the new voter set and X's unregistration results in unregistering a voter
!CVS.contains(X) && !UVS.contains(X): can unregister, because X is definitely not a voter

So long as UVS is present, X's unregistration request validity only depends on that whether or not X is contained in UVS. This also applies to the registration check that is needed for feature upgrades. If there is an uncommitted voter set, all of that set's voters must be registered to the cluster in order to upgrade a feature to a certain value, since those FeatureLevelRecords will be committed after the uncommitted voter set.

Out of scope (for now)

Concurrent AddVoter and UnregisterController can lead to a voter set member being unregistered. This is because the KRaft voter set history that the controller layer can access lags slightly behind the BatchAccumulator. Given a starting voter set CVS that does not contain the observer controller X, it is possible for X to be unregistered at offset N and then added to the voter set at offset N' where N' > N. In this case, the consequence is the same as before this PR, where controller X would see its unregistration and then register again.

There is not a great solution for this without leaking metadata state to KRaft. The interleaved requests are also unlikely to occur, since UnregisterController is likely to be sent alongside RemoveVoter, not AddVoter. This interleaving with RemoveVoter cannot result in an unregistered controller in the voter set.

Testing

Unit testing
Integration tests in KRaftClusterTest

Reviewers: Jun Rao junrao@gmail.com

junrao

@kevin-wu24 : Thanks for the PR. Left a few comments.

junrao · 2026-08-20T17:30:45Z

     *   <li>{@link org.apache.kafka.common.errors.InvalidRequestException}
-     *   If the request tries to unregister the current active controller id.</li>
+     *   If the request tries to unregister the current active controller id or a controller id
+     *   which is part of the voter set.</li>


The latter covers the former. So, we can just say "If controller id is part of the voter set." Ditto in Controller.

junrao · 2026-08-20T17:35:00Z

+import java.util.function.Supplier;
+
+/**
+ * Checks if the provided node id is a voter according to the raft client.


Hmm, this comment seems misplaced. There is no node id provided to the supplier.

junrao · 2026-08-20T18:01:07Z

        int nodeId,
        Map<String, VersionRange> localSupportedFeatures,
-        List<Integer> quorumNodeIds
+        Supplier<Set<Integer>> votersSupplier


Does the mean Feature update is broken without this PR in dynamic quorum? In dynamic quorum, the passed in quorumNodeIds is empty and that list is used in the following code for feature verifciation.

FeatureControlManager.reasonNotSupported (:360-362) for (int id : quorumFeatures.quorumNodeIds()) { if (!foundControllers.contains(id)) return Optional.of("controller " + id + " has not registered, ..."); }

Does the mean Feature update is broken without this PR in dynamic quorum?

I'm not sure if I would use the word broken. This behavior has existed since dynamic quorum was released, so you can argue it is not a regression. controller.quorum.voters, whose ids ultimately populate quorumNodeIds, can be set on a dynamic quorum cluster, just not on a brand new cluster. For example, if you upgrade from kraft.version=0 to kraft.version=1, you can leave controller.quorum.voters as is, since it will be ignored. This check specifically blocks feature upgrades if not all of the active controller's controller.quorum.voters members have registered with the node. If that configuration is empty, then this check is a no-op.

junrao · 2026-08-20T18:09:18Z

                        MetadataVersion.MINIMUM_VERSION.featureLevel(),
                        MetadataVersion.latestProduction().featureLevel()));
-                quorumFeatures = new QuorumFeatures(0, localSupportedFeatures, List.of(0));
+                quorumFeatures = new QuorumFeatures(0, localSupportedFeatures, () -> Set.of(0));


This is an existing issue. It's a bit weird to provide a supplier for testing in the production code. It will be useful to at least add a comment.

junrao · 2026-08-20T18:35:12Z

                return true;
            }
-            if (featureControl.isControllerId(nodeId)) {
+            if (featureControl.isVoterId(nodeId)) {


This is an existing issue, isVoterId() is true doesn't mean the voter is registered. Should we rename this method to sth like isNodeIdKnown()?

Yeah, I can rename it. There is only one caller of this method, and I think the only issue with isNodeIdRegistered is that the name is not an accurate description of what the method evaluates.

junrao · 2026-08-20T18:36:25Z

            () -> {
                if (nodeId == controllerId) {
                    throw new InvalidRequestException("Controller cannot unregister itself while it is active.");
+                } else if (featureControl.isVoterId(controllerId)) {


The check before this line now seems redundant.

junrao · 2026-08-20T20:53:44Z

        return nodeId == other.nodeId &&
-            localSupportedFeatures.equals(other.localSupportedFeatures) &&
-            quorumNodeIds.equals(other.quorumNodeIds);
+            localSupportedFeatures.equals(other.localSupportedFeatures);


Could we add a comment that votersSupplier is excluded deliberately?

kevin-wu24 · 2026-08-20T22:51:43Z

@junrao Thanks for the review. I pushed a commit to address your comments. Please take a look when you have time.

junrao

@kevin-wu24 : Thanks for the updated PR. Just a minor comment.

junrao · 2026-08-20T22:54:19Z

         * Checks if a node id is registered as a broker, controller in static/dynamic quorum.
         */
-        private boolean isNodeIdRegistered(int nodeId) {
+        private boolean isNodeIdKnown(int nodeId) {


Could we adjust the comment above too?

junrao

@kevin-wu24 : Thanks for the updated PR. LGTM. Could we update the changes in the original KIP and send a summary to the KIP mailing list?

kevin-wu24 · 2026-08-21T23:10:55Z

@kevin-wu24 : Thanks for the updated PR. LGTM. Could we update the changes in the original KIP and send a summary to the KIP mailing list?

Updated both the KIP and send a message to the mailing list. Thanks again @junrao for the reviews on this feature.

first draft

5a0bdfb

kevin-wu24 mentioned this pull request Aug 13, 2026

KAFKA-20865: Do not unregister KRaft voter set members #23151

Closed

github-actions Bot added core Kafka Broker kraft clients triage PRs from the community labels Aug 13, 2026

kevin-wu24 added 2 commits August 13, 2026 11:44

update voter set membership logic

ff5bcfd

clean up tests

ce49056

kevin-wu24 marked this pull request as ready for review August 13, 2026 17:05

kevin-wu24 added 2 commits August 18, 2026 11:10

clean up

2e2ff84

merging trunk

303a8ed

junrao reviewed Aug 20, 2026

View reviewed changes

code review

763fd90

junrao reviewed Aug 20, 2026

View reviewed changes

junrao added the ci-approved label Aug 20, 2026

updating comment

480c9f7

github-actions Bot removed the triage PRs from the community label Aug 21, 2026

junrao approved these changes Aug 21, 2026

View reviewed changes

junrao merged commit 05f61b9 into apache:trunk Aug 21, 2026
20 checks passed

giuseppelillo mentioned this pull request Sep 4, 2026

[aiven] fix: allow decommissioning stale "ghost" controllers aiven/kafka#70

Merged

Uh oh!

Conversation

kevin-wu24 commented Aug 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Out of scope (for now)

Testing

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin-wu24 commented Aug 20, 2026

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevin-wu24 commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kevin-wu24 commented Aug 13, 2026 •

edited by github-actions Bot

Loading

junrao Aug 20, 2026 •

edited

Loading