cmccabe commented on code in PR #14838:
URL: https://github.com/apache/kafka/pull/14838#discussion_r1414710963
##########
metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java:
##########
@@ -354,6 +356,25 @@ public ControllerResult<BrokerRegistrationReply>
registerBroker(
throw new BrokerIdNotRegisteredException("Controller is in
pre-migration mode and cannot register KRaft " +
"brokers until the metadata migration is complete.");
}
+
+ if (featureControl.metadataVersion().isDirectoryAssignmentSupported())
{
+ Set<Uuid> set = new HashSet<>(request.logDirs());
+ if (set.stream().anyMatch(DirectoryId::reserved)) {
+ throw new DuplicateBrokerRegistrationException("Reserved
directory ID in request");
Review Comment:
Hmm. `DuplicateBrokerRegistrationException` is certainly the wrong thing,
but I don't think `BrokerIdNotRegisteredException` is quite right either. That
exception was intended for when you made an RPC that required broker N to be
registered (like heartbeat) but broker N was not in fact registered. It wasn't
intended for an error returned from the registration RPC itself. After all, it
provides no information. OK the broker is not registered, but why?
I can see that there are some lines above which are using
`BrokerIdNotRegisteredException` as a generic "didn't register" error, but that
doesn't seem quite right...
```
if (request.isMigratingZkBroker() && !zkRegistrationAllowed()) {
throw new BrokerIdNotRegisteredException("Controller does not
support registering ZK brokers.");
}
if (!request.isMigratingZkBroker() &&
featureControl.inPreMigrationMode()) {
throw new BrokerIdNotRegisteredException("Controller is in
pre-migration mode and cannot register KRaft " +
"brokers until the metadata migration is complete.");
}
```
The "pre-migration" thing is just a straightforward logic error that should
never happen. The `request.isMigratingZkBroker()` check probably deserves its
own error code since it's a weird scenario that should have been spelled out in
the KIP.
Your thing is more like INVALID_REQUEST, since someone is sending obvious
nonsense which superficially resembles the schema.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
