MINOR: Cleanups in coordinator-common/group-coordinator by mimaison · Pull Request #20097 · apache/kafka

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

    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

    /**
     * @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

-        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

        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

-        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]>

The server-side assignors are designed to re-use the previous assignment maps where possible and clone them for modification lazily. apache#20097 introduced a bug where the assignments would always be re-wrapped in an unmodifiable map and so we would end up cloning them repeatedly in the assignors when multiple changes need to be made to a member assignment. Fix the bug by removing the unmodifiable map wrapping.

) The server-side assignors are designed to re-use the previous assignment maps where possible and clone them for modification lazily. #20097 introduced a bug where the assignments would always be re-wrapped in an unmodifiable map and so we would end up cloning them repeatedly in the assignors when multiple changes need to be made to a member assignment. Fix the bug by removing the unmodifiable map wrapping. Reviewers: David Jacot <[email protected]>

…che#21058) The server-side assignors are designed to re-use the previous assignment maps where possible and clone them for modification lazily. apache#20097 introduced a bug where the assignments would always be re-wrapped in an unmodifiable map and so we would end up cloning them repeatedly in the assignors when multiple changes need to be made to a member assignment. Fix the bug by removing the unmodifiable map wrapping. Reviewers: David Jacot <[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

squah-confluent mentioned this pull request Dec 2, 2025

KAFKA-19909: Add rack awareness assignment to UniformHomogeneousAssignmentBuilder #20000

Open

squah-confluent mentioned this pull request Dec 2, 2025

KAFKA-19955: Fix performance regression in server-side assignors #21058

Merged

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mimaison commented Jul 3, 2025 •

edited by github-actions Bot

Loading

Yunyung Jul 7, 2025 •

edited

Loading