MINOR: Refactor GroupMetadataManagerTest#15348
Conversation
| files="(ConsumerGroupMember|GroupMetadataManager|GeneralUniformAssignmentBuilder).java"/> | ||
| <suppress checks="(NPathComplexity|MethodLength)" | ||
| files="(GroupMetadataManager|ConsumerGroupTest|GroupMetadataManagerTest|GeneralUniformAssignmentBuilder).java"/> | ||
| files="(GroupMetadataManager|ConsumerGroupTest|GroupMetadataManagerTest|GroupMetadataManagerTestContext|GeneralUniformAssignmentBuilder).java"/> |
There was a problem hiding this comment.
what's your take on in file suppressions vs suppressions.xml?
On a previous PR I was asked to make the supppression in file for specific methods: #15139 (comment)
There was a problem hiding this comment.
I was actually not aware of this. I think that it makes sense. I file https://issues.apache.org/jira/browse/KAFKA-16244 to give it a try. In this PR, I would prefer to keep the suppressions.xml exception though.
|
|
||
| public class GroupMetadataManagerTestContext { | ||
|
|
||
| public static void assertResponseEquals( |
There was a problem hiding this comment.
Any reason this is here rather than the Assertions class?
Generally, I noticed there were a few static methods from the test that were moved into this class. But it seems like they don't need to be and could be in a general utils class. There are also a few static classes. Is the main idea that we don't move classes out unless they are shared?
Or we plan to do in a followup?
There was a problem hiding this comment.
I agree with @jolshan this class seems to have some methods that are just assertions that maybe better to move to Assertions.java. I got that this pr isn't changing the tests themselves but moving these to the right place wouldn't change the tests and keep things clear specially that these methods didn't belong to any of the static classes before.
There was a problem hiding this comment.
Basically, I moved methods used by at least two other classes to Assertions. I kept the others as private where they are used.
| final private MockCoordinatorTimer<Void, Record> timer = new MockCoordinatorTimer<>(time); | ||
| final private LogContext logContext = new LogContext(); | ||
| final private SnapshotRegistry snapshotRegistry = new SnapshotRegistry(logContext); | ||
| final private TopicPartition groupMetadataTopicPartition = new TopicPartition("topic", 0); |
There was a problem hiding this comment.
groupMetadataTopicPartition doesn't seem to be used anywhere
| return timeouts; | ||
| } | ||
|
|
||
| public MockCoordinatorTimer.ScheduledTimeout<Void, Record> assertSessionTimeout( |
There was a problem hiding this comment.
The return value here doesn't seem to be used by any of the tests can't we just return void?
| } | ||
|
|
||
| public List<ListGroupsResponseData.ListedGroup> sendListGroups(List<String> statesFilter, List<String> typesFilter) { | ||
| Set<String> statesFilterSet = statesFilter.stream().collect(Collectors.toSet()); |
There was a problem hiding this comment.
Can't these be converted to be new HashSet(statesFilter) instead of streaming and collecting
|
|
||
| public List<ListGroupsResponseData.ListedGroup> sendListGroups(List<String> statesFilter, List<String> typesFilter) { | ||
| Set<String> statesFilterSet = statesFilter.stream().collect(Collectors.toSet()); | ||
| Set<String> typesFilterSet = typesFilter.stream().collect(Collectors.toSet()); |
| if (assignment == null) return null; | ||
|
|
||
| Map<Uuid, Set<Integer>> assignmentMap = new HashMap<>(); | ||
| assignment.forEach(topicPartitions -> { |
There was a problem hiding this comment.
there're few places in the pr where statement lambda can be converted to expression lambda instead. would be nice cleanup
OmniaGM
left a comment
OmniaGM
left a comment
There was a problem hiding this comment.
Thanks for the update. Just some small cleanups but not a blocker for the PR.
| if (assignment == null) return null; | ||
|
|
||
| Map<Uuid, Set<Integer>> assignmentMap = new HashMap<>(); | ||
| assignment.forEach(topicPartitions -> { |
There was a problem hiding this comment.
This also can be expression lambda
| List<ConsumerGroupCurrentMemberAssignmentValue.TopicPartitions> assignment | ||
| ) { | ||
| Map<Uuid, Set<Integer>> assignmentMap = new HashMap<>(); | ||
| assignment.forEach(topicPartitions -> { |
There was a problem hiding this comment.
same could be an expression lambda
| List<Record> records = new ArrayList<>(); | ||
|
|
||
| // Add subscription records for members. | ||
| members.forEach((memberId, member) -> { |
| // Add subscription metadata. | ||
| if (subscriptionMetadata == null) { | ||
| subscriptionMetadata = new HashMap<>(); | ||
| members.forEach((memberId, member) -> { |
| records.add(RecordHelpers.newGroupEpochRecord(groupId, groupEpoch)); | ||
|
|
||
| // Add target assignment records. | ||
| assignments.forEach((memberId, assignment) -> { |
| records.add(RecordHelpers.newTargetAssignmentEpochRecord(groupId, assignmentEpoch)); | ||
|
|
||
| // Add current assignment records for members. | ||
| members.forEach((memberId, member) -> { |
`GroupMetadataManagerTest` class got a little under control. We have too many things defined in it. As a first steps, this patch extracts all the inner classes. It also extracts all the helper methods. However, the logic is not changed at all. Reviewers: Omnia Ibrahim <o.g.h.ibrahim@gmail.com>, Justine Olshan <jolshan@confluent.io>
`GroupMetadataManagerTest` class got a little under control. We have too many things defined in it. As a first steps, this patch extracts all the inner classes. It also extracts all the helper methods. However, the logic is not changed at all. Reviewers: Omnia Ibrahim <o.g.h.ibrahim@gmail.com>, Justine Olshan <jolshan@confluent.io>
`GroupMetadataManagerTest` class got a little under control. We have too many things defined in it. As a first steps, this patch extracts all the inner classes. It also extracts all the helper methods. However, the logic is not changed at all. Reviewers: Omnia Ibrahim <o.g.h.ibrahim@gmail.com>, Justine Olshan <jolshan@confluent.io>
3 participants
GroupMetadataManagerTestclass got a little under control. We have too many things defined in it. As a first steps, this patch extracts all the inner classes. It also extracts all the helper methods. However, the logic is not changed at all.Committer Checklist (excluded from commit message)