fapifta commented on PR #4934: URL: https://github.com/apache/ozone/pull/4934#issuecomment-1598538111
@ashishkumar50 @Galsza I wanted to chime into the locking related discussion, but as I see it is already sorted out, let me tell it upfront, I am fine with either using synchronized or using a read/write lock, and whichever you agree on, I accept that. Two consideration that I would like to speak out loud here: - having a read/write lock makes the code a bit more longer, and with that it increases the time needed to understand what is happening and why, so I have no particular preference for read/write locks if the anticipated load on the endpoint is low. - This endpoint will not handle large traffic ever, and even if there might be a contention once services start to poll regularly this API, it does not block anything else, and all the clients are prepared to wait/retry so it is not a big deal. (Also the client side is not in a performance critical path either at the moment, especially as the clients are caching the data, and start off from the cached data, then refresh the cache in the background.) Considering the anticipated traffic, and the expected performance of this server side endpoint I think having a read/write lock is probably more than we need, as locking here seems to be a potential problem only because we are converting the certificates from X509Certificate to PEM encoded string within the lock. That should not be happening I believe, but to avoid that will require some further changes that are not in scope for this PR I believe. I am happy to hear your opinion, I wanted to share mine as once @Galsza asked me what he should do with concurrent access here, and I suggested that using synchronized would be enough, I wanted to detail why hoping to see if I can learn something I have not thought about and also I would like to understand your point of view. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
