KAFKA-14500; [3/N] add GroupMetadataKey/Value record helpers

[jeffkbkim]

This PR enables the new group metadata manager to generate GroupMetadataKey/Value records.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[dajac]

@jeffkbkim Thanks for the patch. I left a few suggestions.

[dajac]

Any reason why we can't get the assignments from the group?

[jeffkbkim]

the assignment comes directly from the sync group request, i don't think we need to store the assignment inside the group

[dajac]

hmm... don't we store it in MemberMetadata in the current implementation? We set it in setAndPropagateAssignment. I think that we need it because we need the ability to provide it the the member at any time.

[jeffkbkim]

i think you're referring to https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala#L623

the in-memory state is updated after we successfully append to the log which so we won't have this information when we generate the records to append, no?

[dajac]

yeah, that's right. the generic group is a bit weird from this regard because it does not use timeline data structures internally so we could just update it as well (like we do for subscriptions for instance). the part which is weird here is that we take everything from the group but the assignment.

[jeffkbkim]

i think you're saying that ideally we want to create a record solely from the group i.e. update the group with the assignment and pass it into this method. However, we can't because we don't use timeline data structures that could revert if append fails. is my understanding correct?

[dajac]

it is actually the other way around, the generic group is mutable so we could mutate it whenever we want here, i think.

[dajac]

Should we add the following test:

• A unit test for the tombstone method;
• A unit test which triggers the IllegalStateException exceptions;
• Unit tests which verifies the various record versions? It could be a parameterised one.

[jeffkbkim]

i updated testNewGroupMetadataRecord for 3). can you take a look?

[jeffkbkim]

i'm seeing

Class Data Abstraction Coupling is 31 (max allowed is 25) classes [ApiMessageAndVersion, ClientAssignor, ConsumerGroupCurrentMemberAssignmentKey, ConsumerGroupCurrentMemberAssignmentValue, ConsumerGroupCurrentMemberAssignmentValue.TopicPartitions, ConsumerGroupMember.Builder, ConsumerGroupMemberMetadataKey, ConsumerGroupMemberMetadataValue, ConsumerGroupMemberMetadataValue.Assignor, ConsumerGroupMetadataKey, ConsumerGroupMetadataValue, ConsumerGroupPartitionMetadataKey, ConsumerGroupPartitionMetadataValue, ConsumerGroupPartitionMetadataValue.TopicMetadata, ConsumerGroupTargetAssignmentMemberKey, ConsumerGroupTargetAssignmentMemberValue, ConsumerGroupTargetAssignmentMemberValue.TopicPartition, ConsumerGroupTargetAssignmentMetadataKey, ConsumerGroupTargetAssignmentMetadataValue, GenericGroup, GenericGroupMember, GroupMetadataKey, GroupMetadataValue, GroupMetadataValue.MemberMetadata, LinkedHashMap, LogContext, MockTime, Protocol, Record, TopicMetadata, VersionedMetadata]. [ClassDataAbstractionCoupling]

where this suggests from https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/metrics/ClassDataAbstractionCouplingCheck.html#:~:text=Generally%20speaking%2C%20any%20data%20type,the%20structure%20of%20the%20class

Generally speaking, any data type with other data types as members or local variable that is an instantiation (object) of another class has data abstraction coupling (DAC). The higher the DAC, the more complex the structure of the class.

Can we suppress this error?

[dajac]

i'm seeing

Class Data Abstraction Coupling is 31 (max allowed is 25) classes [ApiMessageAndVersion, ClientAssignor, ConsumerGroupCurrentMemberAssignmentKey, ConsumerGroupCurrentMemberAssignmentValue, ConsumerGroupCurrentMemberAssignmentValue.TopicPartitions, ConsumerGroupMember.Builder, ConsumerGroupMemberMetadataKey, ConsumerGroupMemberMetadataValue, ConsumerGroupMemberMetadataValue.Assignor, ConsumerGroupMetadataKey, ConsumerGroupMetadataValue, ConsumerGroupPartitionMetadataKey, ConsumerGroupPartitionMetadataValue, ConsumerGroupPartitionMetadataValue.TopicMetadata, ConsumerGroupTargetAssignmentMemberKey, ConsumerGroupTargetAssignmentMemberValue, ConsumerGroupTargetAssignmentMemberValue.TopicPartition, ConsumerGroupTargetAssignmentMetadataKey, ConsumerGroupTargetAssignmentMetadataValue, GenericGroup, GenericGroupMember, GroupMetadataKey, GroupMetadataValue, GroupMetadataValue.MemberMetadata, LinkedHashMap, LogContext, MockTime, Protocol, Record, TopicMetadata, VersionedMetadata]. [ClassDataAbstractionCoupling]

where this suggests from https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/metrics/ClassDataAbstractionCouplingCheck.html#:~:text=Generally%20speaking%2C%20any%20data%20type,the%20structure%20of%20the%20class

Generally speaking, any data type with other data types as members or local variable that is an instantiation (object) of another class has data abstraction coupling (DAC). The higher the DAC, the more complex the structure of the class.

Can we suppress this error?

It is the first time I see this. I see that we can suppress it.

[jeffkbkim]

@dajac this is ready for review

[dajac]

@jeffkbkim I left two small comments. We can merge it when they are addressed.

[dajac]

LGTM