divijvaidya commented on PR #13435: URL: https://github.com/apache/kafka/pull/13435#issuecomment-1479202476
Hey @Stephan14 Thank you for your change. First, I would like to clarify the motivation of this change. The current code is not unsafe (please correct me if I am wrong) since the code you are changing (`updateStaticMemberAndRebalance()`) is already invoked after acquiring a lock on group. See: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala#L248 Hence, please update the PR title to, "Add additional safety lock in GroupCoordinator when persisting metadata". Second, do we need to add this additional safety lock for a private method? Note that even though it is a re-entrant lock, there is a cost involved in checking for a inside a private method. I am of the opinion that if a lock is acquired at the public method at the very top, then we don't need to acquire re-entrant locks in each of the private methods that it calls. Hence, I don't think we should make this change. However, we should probably add a comment for `updateStaticMemberAndRebalance()` that it should be called after acquiring a lock on group object. -- 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
