Hi José, Thanks for the reply.
RE JS1: I like the idea of a separate `CONTROLER_ID_NOT_REGISTERED` error code for unregistering a controller which is not registered. I have updated the KIP with this. RE JS2: Another case where reusing the ApiKey 1 metadata record may not be a good idea is for a combined node, where the broker and controller share the same node id. When the controller replays this record, should it unregister the broker or the controller? The answer is not super obvious. I think the only way to distinguish between the broker and controller un-registrations is by looking if the `brokerEpoch` is set in the record, but that seems less intuitive than introducing a separate record. I have updated the KIP with this case too. Best, Kevin Wu On Tue, May 12, 2026 at 10:41 AM Jun Rao via dev <[email protected]> wrote: > Hi, Jose, > > Thanks for the reply. > > JS1. I agree that returning BROKER_ID_NOT_REGISTERED for an > UnregisterControllerRequest is inconsistent. Renaming > BROKER_ID_NOT_REGISTERED to NODE_ID_NOT_REGISTERED introduces a change in > the existing protocol and we probably need to bump up > the UnregisterBrokerRequest version to reflect this change. Another way is > to introduce a new CONTROLLER_ID_NOT_REGISTERED error code. > Also, "NodeIdNotRegisteredException extends BrokerIdNotRegisteredException" > seems unintuitive. A node is more general than a broker. So, it seems > that BrokerIdNotRegisteredException should extend > NodeIdNotRegisteredException, not the other way. > > Jun > > On Tue, May 12, 2026 at 5:16 AM José Armando García Sancio via dev < > [email protected]> wrote: > > > Hi Kevin, thanks for the new feature. After introducing KIP-853, we > > need to allow the user to unregister the controller from the cluster. > > > > JS1 > > > a BROKER_ID_NOT_REGISTERED error if no registration exists for the > > requested controller ID > > > > Is it worth cleaning up this error in this KIP? The error name is not > > part of the public API but the Java exception is part of the public > > API. How about: > > 1. Rename BROKER_ID_NOT_REGISTERED to NODE_ID_NOT_REGISTERED > > 2. Add this exception "NodeIdNotRegisteredException extends > > BrokerIdNotRegisteredException". The admin client will always throw > > NodeIdNotRegisteredException but legacy application can still catch > > and handle BrokerIdNotRegisteredException. > > > > JS2 > > > Use UnregisterBrokerRecord to unregister controllers > > > > You could reuse the metadata record with API key 1 to implement this > > feature. The active controller would have to check that the finalized > > metadata version support this feature before writing this record with > > a controller id. Since the name UnregisterBrokerRecord is not part of > > the public API, the implementation can rename this record to > > UnregisterNodeRecord and rename the field brokerId to nodeId. Since > > controllers currently lack an epoch, this field can be ignore for > > controller unregistration. If you want to clean up that field you can > > make it a tagged field and introduce version 1 of the > > UnregisterNodeRecord record. > > > > What do you think? > > -- > > -José > > >
