MINOR: Clean up group instance id handling in GroupCoordinator#9958
MINOR: Clean up group instance id handling in GroupCoordinator#9958hachikuji merged 4 commits intoapache:trunkfrom
GroupCoordinator#9958Conversation
|
cc @dajac No hurry, but if you have time, please take a look. |
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
@hachikuji Great refactoring! It really improve the readability of the code. It seems to me that all the invariants are preserved so it looks good overall. I have left a couple of minor comments. I need to make a second pass over it.
hachikuji
force-pushed
the
clean-up-static-member-handling
branch
from
e668563 to
8bef3bb
Compare
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
@hachikuji Thanks for the updates. I went through the logic again and it looks good to me. I have left few minor comments.
| // get the set of protocols that are commonly supported by all members | ||
| val numMembers = members.size | ||
| supportedProtocols.filter(_._2 == numMembers).map(_._1).toSet | ||
| supportedProtocols.filter(_._2 == numMembers).keys.toSet |
There was a problem hiding this comment.
nit: Could we directly use keysSet instead of .keys.toSet?
There was a problem hiding this comment.
I tried this, but I think we still need the toSet because we are converting to the immutable Set type.
| def remove(memberId: String): Unit = { | ||
| members.remove(memberId).foreach { member => | ||
| member.supportedProtocols.foreach{ case (protocol, _) => supportedProtocols(protocol) -= 1 } | ||
| member.supportedProtocols.foreach { case (protocol, _) => supportedProtocols(protocol) -= 1 } |
There was a problem hiding this comment.
I have noticed that we update supportedProtocols in multiple places with the exact same line (modulo +/-). I think that we should consolidate them into one or two helper methods. We can address this separately though. I just wanted to point that out.
There was a problem hiding this comment.
Yeah, I agree. I tried a few options for this, but I could not find one I felt satisfied with. If you have a good suggestion, I'm happy to give it a shot here or to help review if you want to do a separate PR.
2 participants
This is a continuation of a refactor started in #9952. The logic in
GroupCoordinatoris loose and inconsistent in the handling of thegroupInstanceId. In some cases, such as in the JoinGroup hander, we verify that the groupInstanceId from the request is mapped to the memberId precisely. In other cases, such as Heartbeat, we check the mapping, but only to validate fencing. The patch consolidates the member validation so that all handlers follow the same logic.A second problem is that many of the APIs where a
groupInstanceIdis expected use optional arguments. For example:If
groupInstanceIdisNone, thenhasStaticMemberis guaranteed to returnfalsewhileaddStaticMemberraises anIllegalStateException. So the APIs suggest a generality which is not supported and does not make sense.Finally, the patch attempts to introduce stronger internal invariants inside
GroupMetadata. Currently it is possible for an inconsistentgroupInstanceIdtomemberIdmapping to exist because we expose separate APIs to modifymembersandstaticMembers. We rely on the caller to ensure this doesn't happen. Similarly, it is possible for a member to be in thependingMembersset as well as the stablemembersmap. The patch fixes this by consolidating the paths to addition and removal from these collections and adding assertions to ensure that invariants are maintained.Committer Checklist (excluded from commit message)