Hi Jun, Thanks for your comments. Please find my answers below.
60. Yes, we need both. PartitionAssignor.onAssignment is here to inform the customer assignor about the assignment decision taken with the full set of assigned partitions regardless of whether they are already revoked or not and the custom metadata. Streams needs this in order to act on the new assignment. It will be called only once per epoch. I do agree with the consistency point. I already removed topic ids from the PartitionAssignor interface and I have switched to using Collection instead of List. 61. Done. 62. Make sense. I have standardized on `Collection<TopicPartition> targetPartitions`. 63. `error` is correct here. A member passes a `reason` to the assignor where the `reason` represents the reason of the assignment trigger. The assignor passes an `error` to the member where the `error` represents why the assignment failed. Note that it seems that you looked at a slightly outdated version of the KIP because `MemberAssignment.error` is no longer there. I have chosen to pass `error` to `onAssignment` directly because it allows us to better reuse other classes. Let me know what you think. 64. I like the idea. Let me change `group.remote.assignor` to default to null and this would signal that the default on the coordinator side should be used. The default would be the first one in the list. 65. I think that both are actually valid here. It mainly depends on whether we want to move foo-2 when A revokes it and when B heartbeats next. Let me follow your suggestion as it seems a little clearer in the example. 66. It is actually correct. It is important to transition C to epoch 23 at this stage because the new epoch must be given in the JoinGroup response. The SyncGroup will just collect the assignment. I have added a note to explain this in the example. 67. ConsumerGroupHeartbeatRequest.RebalanceTimeoutMs is defined based on max.poll.interval.ms. It is used in the revocation process where the coordinator gives the rebalance timeout to the member to revoke partitions. It is also used to put an upper bound on the client-side assignment computation. I do agree that the name is not so good now. I wonder if we should just use MaxPollIntervalMs now. What do you think? 68. 68.1 Yes. 68.2 I don't think that we need it at the moment. The error here is a client-side assignor implementation details. It is supposed to mean something in this context so storing it on the server side seems wasteful to me. 69. There is ConsumerGroupPrepareAssignmentResponse.AssignorName. All the ConsumerGroupPrepareAssignmentResponse.Members.Assignor are of this type. 70. ConsumerGroupInstallAssignmentRequest.GroupEpoch is supposed to be set based on ConsumerGroupPrepareAssignmentResponse.GroupEpoch. I have added this for fencing purposes. It will allow the group coordinator to reject an installation if the group epoch does not match the expected one. I do agree that the client should not do anything but to copy the value here. 71. 71.1 Hum.. I am not sure about this one. I tend to agree that it would make the life of the tools simpler. However, there is also `SubscribedTopicIds`. Should we add the names for all of them? Or perhaps, provide a mapping from ID to name in the response. The mapping may be the best option here in order to not repeat names everywhere. What do you think? 71.2 Totally 72. It should be optional. Members are expected to set it and the coordinator will use them to fence them if the epoch is not correct. Tools can omit it. We have a similar behavior for the OffsetCommitRequest so in a sense we make the OffsetFetchRequest consistent with its counterpart. Best, David On Thu, Oct 13, 2022 at 6:21 PM Jun Rao <[email protected]> wrote: > > Hi, David, > > Thanks for the reply and the updated KIP. A few more comments on the > interfaces and the protocols. > > 60. On the consumer side, do we need both PartitionAssignor.onAssignment > and ConsumerRebalanceListener.onPartitionsAssigned? My understanding is > that the former was added for cooperative rebalance, which is now handled > by the coordinator. If we do need both, should we make them more consistent > (e.g. topic name vs topic id, list vs set vs collection)? > > 61. group.local.assignors: Could we make it clear that it's the full class > name that implements PartitionAssignor? > > 62. MemberAssignment: It currently has the following method. > public Set<TopicPartition> topicPartitions() > We are adding List<TopicIdPartition> ownedPartitions. Should we keep the > naming and the return type consistent? > > 63. MemberAssignment.error: should that be reason? > > 64. group.remote.assignor: The client may not know what assignors the > broker supports. Should we default this to what the broker determines (e.g. > first assignor listed in group.consumer.assignors)? > > 65. After the text "When A heartbeats again and acknowledges the > revocation, the group coordinator transitions him to epoch 2 and releases > foo-2.", we have the following. > B - epoch=2, partitions=[foo-2], pending-partitions=[] > Should foo-2 be in pending-partitions? > > 66. In the Online Migration example, is the first occurence of "C - > epoch=23, partitions=[foo-2, foo-5, foo-4], pending-partitions=[]" correct? > It seems that should happen after C receives a SyncGroup response? If so, > subsequent examples have the same issue. > > 67. ConsumerGroupHeartbeatRequest.RebalanceTimeoutMs : Which config > controls this? How is this used by the group coordinator since there is no > sync barrier anymore? > > 68. ConsumerGroupHeartbeatResponse: > 68.1 AssignedTopicPartitions and PendingTopicPartitions are of type > []TopicPartition. Should they be TopicPartitions? > 68.2 Assignment.error. Should we also have an errorMessage field? > > 69. ConsumerGroupPrepareAssignmentResponse.Members.Assignor: Should it > include the selected assignor name? > > 70. ConsumerGroupInstallAssignmentRequest.GroupEpoch: Should we let the > client set this? Intuitively, it seems the coordinator should manage the > group epoch. > > 71. ConsumerGroupDescribeResponse: > 71.1 Members.Assignment.Partitions. Should we include the topic name too > since it's convenient for building tools? Ditto for TargetAssignment. > 71.2 Members: Should we include SubscribedTopicRegex too? > > 72. OffsetFetchRequest: Is GenerationIdOrMemberEpoch needed since tools may > also want to issue this request? > > Thanks, > > Jun
