dajac commented on code in PR #20714:
URL: https://github.com/apache/kafka/pull/20714#discussion_r2438834831
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetMetadataManager.java:
##########
@@ -632,6 +618,17 @@ public CoordinatorResult<OffsetCommitResponseData,
CoordinatorRecord> commitOffs
.setPartitionIndex(partition.partitionIndex())
.setErrorCode(Errors.OFFSET_METADATA_TOO_LARGE.code()));
} else {
+ // Validate per-partition commit if needed
+ if (requiresPartitionValidation) {
+ group.validateOffsetCommitForPartition(
+ request.memberId(),
+ request.generationIdOrMemberEpoch(),
+ topic.name(),
+ topic.topicId(),
+ partition.partitionIndex()
+ );
Review Comment:
This may be a bit inefficient because we have to lookup the member for every
partition. I wonder whether we could do something as follow.
Let's define an OffsetPartitionValidator. We could find a better name.
```
OffsetPartitionValidator {
void validate(name, topicId, partitionId)
}
```
At the group level, we can have a method which return the a concrete
OffsetPartitionValidator depending on the member epoch / group type. If the
epoch matches the expected epoch, the validator is basically a no op.
Otherwise, we return one which does the partition level check while caching the
reference to the member.
```
Group {
OffsetPartitionValidator offsetValidator(memberId, memberEpoch)
}
```
This would make the validation logic in this class kind of agnostic of the
two level validation.
Have you considered something like this? What do you think about it?
--
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]
