[GitHub] [kafka] cmccabe commented on pull request #10550: MINOR: Add support for ZK Authorizer with KRaft

[GitBox <gi...@apache.org>]

cmccabe commented on pull request #10550:
URL: https://github.com/apache/kafka/pull/10550#issuecomment-839281487


   Thanks for working on this, @rondagostino , and sorry about the delays in reviewing.
   
   > We forward the Create/Remove operations to the controller, but this patch actually short-circuits that if we are using KRaft with the ZooKeeper-based `AclAuthorizer` via the changes to `RaftSupport.maybeForward()`. The reason for short-circuiting it is because the KRaft controller doesn't have the code to create or remove ACLs (`handle{Create,Delete}Acls` in `KafkaApis`). We could add it, of course, in which case the changes to the `maybeForward()` method would be unnecessary. Perhaps it would be simpler to do that instead of delaying it to an additional PR -- is that what you were suggesting?
   
   Yes, I think we should just do this in `ControllerApis.scala` and be done with it.  It's kind of annoying to do in a follow-on PR since we'd have to add a lot of special-case code which we'd later have to undo, which is not usually the right way to go.  We might even be able to move this code into `RequestHandlerHelper` or something since it would be the same between `BrokerApis` and `ControllerApis` (I think?)


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org