MINOR: Cleanups in coordinator-common/group-coordinator #20097

mimaison · 2025-07-03T15:23:51Z

Use record where possible
Use enhanced switch
Tweak a bunch of assertions

Reviewers: Yung [email protected], TengYao Chi
[email protected], Ken Huang [email protected], Dongnuo Lyu
[email protected], PoAn Yang [email protected]

Yunyung

Overall, LGTM. Left one minor comment.

Yunyung · 2025-07-04T13:29:59Z

...p-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/TopologyMetadata.java

@@ -36,7 +36,7 @@
 public record TopologyMetadata(MetadataImage metadataImage, SortedMap<String, ConfiguredSubtopology> subtopologyMap) implements TopologyDescriber {

    public TopologyMetadata {
-        metadataImage = Objects.requireNonNull(metadataImage);
+        Objects.requireNonNull(metadataImage);
        subtopologyMap = Objects.requireNonNull(Collections.unmodifiableSortedMap(subtopologyMap));


Nitpick. The same applies to line36 in StreamsGroupHeartbeatResult.java.

Suggested change

subtopologyMap = Objects.requireNonNull(Collections.unmodifiableSortedMap(subtopologyMap));

subtopologyMap = Collections.unmodifiableSortedMap(Objects.requireNonNull(subtopologyMap));

I'm not sure I understand your comment. Can you clarify what you mean?

If the passed subtopologyMap is null
Current: Collections.unmodifiableSortedMap throws NPE (so Objects.requireNonNull never takes effect).
After: Objects.requireNonNull throws NPE.

Also, this results in a different stack trace if NPE.

Good point, done

m1a2st

Thanks @mimaison for this patch, a little comment

m1a2st · 2025-07-07T02:09:39Z

...rdinator/src/main/java/org/apache/kafka/coordinator/group/OffsetExpirationConditionImpl.java

    /**
     * @return The base timestamp.
     */
+    @Override
    public Function<OffsetAndMetadata, Long> baseTimestamp() {
        return this.baseTimestamp;
    }


The override method also can be removed.

Right, done

frankvicky · 2025-07-09T03:47:01Z

...oordinator/src/main/java/org/apache/kafka/coordinator/group/modern/MemberAssignmentImpl.java

-        this.partitions = Objects.requireNonNull(partitions);
+public record MemberAssignmentImpl(Map<Uuid, Set<Integer>> partitions) implements MemberAssignment {
+    public MemberAssignmentImpl {
+        Objects.requireNonNull(partitions);


Should we wrap the partitions with Collections.unmodifiableMap?
This could ensure the consistency of hashCode.

Yup I added that

FrankYang0529

Since the PR does some cleanup in GroupMetadataManagerTest, could we also fix following items in this PR? Thanks

Change to assertEquals(Errors.LEADER_NOT_AVAILABLE, appendGroupMetadataErrorToResponseError(Errors.LEADER_NOT_AVAILABLE));. I think the assertion would like to test appendGroupMetadataErrorToResponseError can return input appendError if error is non-matched.

https://github.com/apache/kafka/blob/dabde76ebf105aaa945db60b7753331c83a8c989/group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java#L6339

Wrong orders.

https://github.com/apache/kafka/blob/dabde76ebf105aaa945db60b7753331c83a8c989/group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java#L6641

https://github.com/apache/kafka/blob/dabde76ebf105aaa945db60b7753331c83a8c989/group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java#L16074

https://github.com/apache/kafka/blob/dabde76ebf105aaa945db60b7753331c83a8c989/group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java#L16088

Unnecessary ;

https://github.com/apache/kafka/blob/dabde76ebf105aaa945db60b7753331c83a8c989/group-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java#L15640

dongnuo123 · 2025-07-14T18:00:54Z

group-coordinator/src/main/java/org/apache/kafka/coordinator/group/Utils.java

@@ -219,7 +219,7 @@ public static Map<Uuid, Set<Integer>> assignmentFromTopicPartitions(
    ) {
        return topicPartitionsList.stream().collect(Collectors.toMap(
            ConsumerGroupCurrentMemberAssignmentValue.TopicPartitions::topicId,
-            topicPartitions -> Collections.unmodifiableSet(new HashSet<>(topicPartitions.partitions()))));
+                topicPartitions -> Collections.unmodifiableSet(new HashSet<>(topicPartitions.partitions()))));


nit: Do we need the extra indentation? since L221 and 222 are two parameters of Collectors.toMap

mimaison · 2025-07-30T14:25:12Z

Thanks @FrankYang0529 for the review. I pushed an update addressing the issues you found.

FrankYang0529

LGTM. Thanks for the patch.

Pankraz76 · 2025-07-31T09:16:20Z

...ommon/src/test/java/org/apache/kafka/coordinator/common/runtime/MockCoordinatorExecutor.java

-        public final String key;
-        public final CoordinatorResult<Void, T> result;
-
+    public record ExecutorResult<T>(String key, CoordinatorResult<Void, T> result) {


Use record where possible (Class can be record class) openrewrite/rewrite-static-analysis#682

Pankraz76 · 2025-07-31T09:19:53Z

Well done thanks. Kind of hard to review as its entangled, but thats how its done, no offence given.

Its not ensured that these patterns wont (re)occur anywhere else, making the whole storing ongoing.

Rewrite has the ability to automate this:

Hope this helps, thanks.

- Use `record` where possible - Use enhanced `switch` - Tweak a bunch of assertions Reviewers: Yung <[email protected]>, TengYao Chi <[email protected]>, Ken Huang <[email protected]>, Dongnuo Lyu <[email protected]>, PoAn Yang <[email protected]>

github-actions bot added the group-coordinator label Jul 3, 2025

mimaison force-pushed the cleanups-coordinator branch from 341e247 to 1d6f7b5 Compare July 4, 2025 09:50

Yunyung reviewed Jul 4, 2025

View reviewed changes

m1a2st reviewed Jul 7, 2025

View reviewed changes

frankvicky reviewed Jul 9, 2025

View reviewed changes

FrankYang0529 reviewed Jul 9, 2025

View reviewed changes

dongnuo123 reviewed Jul 14, 2025

View reviewed changes

mimaison and others added 3 commits July 30, 2025 15:52

MINOR: Cleanups in coordinator-common/group-coordinator

9aab514

Remove OffsetExpirationConditionImpl.baseTimestamp()

625979c

Address comments

bd91f38

mimaison force-pushed the cleanups-coordinator branch from 7c15597 to bd91f38 Compare July 30, 2025 14:16

FrankYang0529 approved these changes Jul 31, 2025

View reviewed changes

FrankYang0529 merged commit fd9b551 into apache:trunk Jul 31, 2025
26 checks passed

mimaison deleted the cleanups-coordinator branch July 31, 2025 08:41

This was referenced Jul 31, 2025

PoC: Add rewrite support for errorprone.refasterrules #20219

Closed

Use record where possible (Class can be record class) openrewrite/rewrite-static-analysis#682

Closed

Pankraz76 reviewed Jul 31, 2025

View reviewed changes

Pankraz76 mentioned this pull request Aug 6, 2025

Add rewrite support for RemoveUnusedImports #20314

Closed

	subtopologyMap = Objects.requireNonNull(Collections.unmodifiableSortedMap(subtopologyMap));
	subtopologyMap = Collections.unmodifiableSortedMap(Objects.requireNonNull(subtopologyMap));

MINOR: Cleanups in coordinator-common/group-coordinator #20097

MINOR: Cleanups in coordinator-common/group-coordinator #20097

Uh oh!

Conversation

mimaison commented Jul 3, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yunyung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yunyung Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankYang0529 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jul 30, 2025

Uh oh!

FrankYang0529 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 commented Jul 31, 2025

Uh oh!

Uh oh!

mimaison commented Jul 3, 2025 •

edited by github-actions bot

Loading

Yunyung Jul 7, 2025 •

edited

Loading