Clarify the behavior in the partition assigner protocol #544
Conversation
|
#231 may need to be reviewed: It added the userData parameter in some places where this PR removes it again from. |
The user data isn't provided to the 'assign' function as field in the argument object, but rather it is part of the 'members' array. This commit adds an integration test showing how to use the user data.
This assigner doesn't use the user data, and with the previous commit is also clear that the data isn't provided as fields in the argument objects, but rather would have been produced here.
ankon
force-pushed
the
pr/clarify-assigner-user-data-behavior
branch
from
8611713
to
354437e
Compare
|
This looks "big", but is actually just typing fixes + a unit test that this actually "works". |
| @@ -59,18 +63,16 @@ module.exports = ({ cluster }) => ({ | |||
| memberAssignment: MemberAssignment.encode({ | |||
There was a problem hiding this comment.
Shouldn't it provide the userData for that member in the MemberAssignment? The only difference is that the assign method gets the userData from each member (as memberMetadata). Maybe I'm missing something.
There was a problem hiding this comment.
The user data is part of the members, encoded into the memberMetadata by the protocol function.
The protocol function decides about the user data (in the RR assigner: nothing), and sends that to the broker as part of the member metadata. The broker then collects the members (and their metadata), and triggers the assign call (for the selected leader). The assign function implementation then can pull out the user data from the provided content.
The flow is indeed a bit odd, and I was arguing with myself here whether to leave some references lying around, but ultimately the RR assigner doesn't use any user data, and having it here is just confusing. Instead I added the unit test to show how to pass user data. Maybe a "how to write your own assigner" article could also help.
|
Sorry that it's taking forever to get this reviewed. I'm on vacation at the moment, and I suspect Tulio has his plate full. |
The existing typing was off, and made it look like the user data would be provided to the
protocolandassignfunctions as argument fields.Reading through https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-GroupMembershipAPI and the KafkaJS code is not true at all. What actually happens is that
protocolfunction can provide user data (by encoding it into the result), thenassignfunction. At this point it can take the user data into account, and it can produce new user data*The first commit in this PR updates the TypeScript types to match this behavior and adds a integration test to demonstrate it, the second commit then cleans up the roundRobinAssigner's code to remove the "obviously unused" argument fields.