TakaHiR07 commented on PR #16792: URL: https://github.com/apache/pulsar/pull/16792#issuecomment-1233971743
> Thanks for your contribution @TakaHiR07. This is a great start. However, I think we'll need some additional work before it is ready to be merged. > > First, I think this work will need to be two PRs. > > There will be a PR to update how permissions are revoked. Essentially, we should not fail a request that attempts to remove permission from a partition of a topic that was not already granted permission. We will need to determine how to handle the case that a non-partitioned topic or all of the partitions of a partitioned topic (including the partitioned topic itself) does not have permission. This PR will be backwards compatible and will therefore be able to be cherry picked to existing release branches. > > Then there is a PR that will change what the `PersistentTopicsBase` passes to the `AuthorizationService`. This PR is an optimization to limit the amount of metadata stored in ZK. Instead of passing each topic partition to the `grantPermissionsAsync` method, we'll just pass the topic that the user is attempting to grant permission on. This will technically be a breaking change for custom implemenations of the `AuthorizationProvider` interface, so we cannot cherry pick this change. > > In order to be upgrade and downgrade compatible, we'll need to leave the revoke permission logic the same for a multiple minor versions. This work could be tracked in an issue. > > Since both PRs will have to do with authorization and what the Pulsar Broker passes to the pluggable `AuthorizationProvider`, I think we should give a notice on the [[email protected]](mailto:[email protected]) mailing list to get consensus. I expect that this will not be a blocker for the feature/bug fix. I agree with it. I open another PR to update how permissions are revoked. This PR remain as the second PR to optimize the implementation of permission. I am not familiar with how to get consensus, so I would wait for update this pr until the implementation is approved. -- 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]
