kevin-wu24 commented on code in PR #22191:
URL: https://github.com/apache/kafka/pull/22191#discussion_r3336214173
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -2128,6 +2132,19 @@ public CompletableFuture<Void> registerController(
EnumSet.noneOf(ControllerOperationFlag.class));
}
+ @Override
+ public CompletableFuture<Void> unregisterController(
+ ControllerRequestContext context,
+ int controllerId
+ ) {
+ if (nodeId == controllerId && isActiveController()) {
Review Comment:
Discussed offline. There is a different issue in that putting this check
outside of the event queue allows for races:
On the request handler thread at time X, the controller is either active or
inactive, and the write event is guaranteed to be handled at some time Y > X.
Assume we have this check before appending the write event.
Inactive case (1) + stay inactive at Y: we can't run the
ControllerWriteEvent and return error.
Inactive case (1) + become active before time Y: it looks like we would have
a race between the handleLeaderChange RaftEvent and the unregister WriteEvent?
The order is:
- check on request handler that passes because not active controller
- handleLeaderChange makes local node the leader before the unregister write
event is handled
- You can unregister the leader now....
Active case (2) + stay active: this is an invalid request at time Y too, so
no issue.
Active case (2) + become inactive: I think we would be returning an error
here, when the request should be "handleable." It's a similar case to the
second.
--
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]
