Copilot commented on code in PR #21558:
URL: https://github.com/apache/kafka/pull/21558#discussion_r2853044428
##########
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/modern/consumer/CurrentAssignmentBuilderTest.java:
##########
@@ -554,16 +561,20 @@ public void
testUnrevokedPartitionsToStableWithReturnedPartitionsPendingRevocati
.setPartitions(Arrays.asList(5, 6))))
.build();
+ // Retained partitions keep original epoch (10), partition 4 was
pending revocation so gets new epoch (12),
Review Comment:
The comment on lines 564-565 states "partition 4 was pending revocation so
gets new epoch (12)", but the expected assignment on line 567 shows partition 4
with epoch 10, not 12. According to the comment and the logic in
`CurrentAssignmentBuilder.computeNextAssignment` lines 452-461, when a
partition returns from pending revocation to assigned, it should retain its
original epoch (10 in this case). The comment should be corrected to say
"partition 4 was pending revocation so keeps its original epoch (10)" to match
the actual behavior and expected values.
```suggestion
// Retained partitions keep original epoch (10), partition 4 was
pending revocation so keeps its original epoch (10),
```
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorRecordHelpers.java:
##########
@@ -804,13 +804,19 @@ public static CoordinatorRecord
newShareGroupStatePartitionMetadataRecord(
}
private static
List<ConsumerGroupCurrentMemberAssignmentValue.TopicPartitions>
toTopicPartitions(
- Map<Uuid, Set<Integer>> topicPartitions
- ) {
- List<ConsumerGroupCurrentMemberAssignmentValue.TopicPartitions> topics
= new ArrayList<>(topicPartitions.size());
- topicPartitions.forEach((topicId, partitions) ->
+ Map<Uuid, Map<Integer, Integer>> topicPartitionsWithEpochs
+ ) {
+ List<ConsumerGroupCurrentMemberAssignmentValue.TopicPartitions> topics
= new ArrayList<>(topicPartitionsWithEpochs.size());
+ topicPartitionsWithEpochs.forEach((topicId, partitionEpochMap) -> {
+ List<Integer> partitionList = new
ArrayList<>(partitionEpochMap.keySet());
Review Comment:
The `partitionList` is created from `partitionEpochMap.keySet()` (line 811),
and then `epochList` is created by mapping each partition in `partitionList` to
its epoch from the map (lines 812-814). However, the order of entries from
`keySet()` for a `HashMap` is not deterministic across different JVM runs or
serialization/deserialization cycles. While the alignment between
`partitionList` and `epochList` will be correct within a single execution, this
could lead to non-deterministic ordering in the serialized records, which may
cause issues with testing, debugging, or record comparison.
Consider using a sorted list or TreeMap to ensure deterministic ordering of
partitions.
```suggestion
List<Integer> partitionList = new
ArrayList<>(partitionEpochMap.keySet());
Collections.sort(partitionList);
```
##########
jmh-benchmarks/src/main/java/org/apache/kafka/jmh/assignor/AssignorBenchmarkUtils.java:
##########
@@ -138,7 +138,7 @@ public static GroupSpec createGroupSpec(
Optional.ofNullable(member.rackId()),
Optional.ofNullable(member.instanceId()),
new TopicIds(member.subscribedTopicNames(), topicResolver),
- new Assignment(member.assignedPartitions())
+ new Assignment(Map.of())
Review Comment:
The assignment is being set to an empty map (`Map.of()`) instead of using
the member's actual assigned partitions. This change appears intentional based
on the PR description which states the structure is being changed, but it means
the benchmark no longer includes the member's actual assignments. This could
affect the accuracy of benchmark results if the assignor's performance is
influenced by existing assignments.
Consider whether benchmarks need to be updated to use the new assignment
structure with epochs, or if starting with empty assignments is intentional for
this benchmark scenario.
```suggestion
member.assignment()
```
--
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]
