lhotari commented on PR #24209: URL: https://github.com/apache/pulsar/pull/24209#issuecomment-2829500677
> > @nodece I didn't review this yet, but just wondering if TenantInfo modifications contain a thread safety issue similar to #21303 which hasn't been addressed for TopicPolices. For NamespacePolicies, the issue was #9711 and that has been addressed. > > @lhotari It doesn't relate to thread safety, the root cause is that the broker didn't correctly handle the null value. This PR also adds a feature that fills the clusters if empty. @nodece Yes, my comment about thread safety was just a reminder that we haven't yet addressed that for TopicPolicies. In the case of TenantInfo, there is no thread safety issue since no object instances or fields are reused when it's updated. However, there's an existing problem about the usability of the API. When you update the tenant, you must provide values for all fields and there's no notation for patching the existing TenantInfo instance. I've recently seen one report from our user where the admin roles got unexpectedly resetted to empty when updating the allowed clusters. In PulsarAdmin CLI, this is handled in the CLI tool code: https://github.com/apache/pulsar/blob/434ec1b884b26af6c3658df3a7b90c432e464973/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTenants.java#L102-L114 This is just a side note and I'm not expecting it to be addressed in this PR. I guess this could be addressed by better documentation of the client API. One possible approach for handling the unexpected reset to empty admin roles would be to only reset the admin roles if an empty list was provided as input, however that would change the existing semantics. I guess a similar problem exists with the allowed clusters. -- 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: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
