kevin-wu24 commented on code in PR #22191:
URL: https://github.com/apache/kafka/pull/22191#discussion_r3290419664
##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -494,12 +503,56 @@ static void handleRemoveController(
throw new TerseException("Failed to parse
--controller-directory-id: " + e.getMessage());
}
if (!dryRun) {
- admin.removeRaftVoter(controllerId, directoryId).
- all().get();
+ removeRaftVoter(admin, controllerId, directoryId, unregister);
}
System.out.printf("%s KRaft controller %d with directory id %s%n",
dryRun ? "DRY RUN of removing " : "Removed ",
controllerId,
directoryId);
+ if (unregister) {
+ if (!dryRun) {
+ unregisterController(admin, controllerId);
+ }
+ System.out.printf("%s KRaft controller %d%n",
+ dryRun ? "DRY RUN of unregistering " : "Unregistered ",
+ controllerId);
+ }
+ }
+
+ private static void removeRaftVoter(
+ Admin admin,
+ int controllerId,
+ Uuid directoryId,
+ boolean unregister
+ ) throws TerseException, ExecutionException, InterruptedException {
+ try {
+ admin.removeRaftVoter(controllerId, directoryId).all().get();
+ } catch (ExecutionException e) {
+ Throwable cause = e.getCause();
+ if (unregister && (cause instanceof UnsupportedVersionException ||
+ cause instanceof VoterNotFoundException)) {
+ throw new TerseException("Failed to remove KRaft voter " +
controllerId
+ + ": " + cause.getMessage()
+ + ". To unregister the controller from the cluster, run "
+ + "`kafka-cluster.sh unregister-controller --controller-id
"
+ + controllerId + "`.");
Review Comment:
> hm, should we include the original exception so the client knows why
removeRaftVoter failed?
Throwing `TerseException` strips the stack trace, but that only occurs when
the user tries to remove AND unregister the controller in the same command. In
any case, we still print `cause.getMessage()`, so we'll see Kafka's message
when initializing the exception IIUC.
For these two exceptions specifically, I wanted to have the ability to
direct the user to run the other tool if they want to unregister the
controller, since removing the voter is not possible if those exceptions are
returned.
> also, if the removal fails due to UnsupportedVersionException then
wouldn't unregisterController also be unsupported?
`UnsupportedVersionException` being returned by `RemoveRaftVoterRequest`
means the cluster is kraft.version=0. You could still unregister a controller
in a static quorum cluster, since kraft.version is a different feature from MV.
For example, you can have an observer that does not set
`controller.quorum.voters` and instead sets
`controller.quorum.bootstrap.servers`.
--
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]
