Copilot commented on code in PR #20702:
URL: https://github.com/apache/kafka/pull/20702#discussion_r2434710922
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -5419,6 +5429,15 @@ public void replay(
streamsGroup.setGroupEpoch(value.epoch());
streamsGroup.setMetadataHash(value.metadataHash());
streamsGroup.setValidatedTopologyEpoch(value.validatedTopologyEpoch());
+
+ streamsGroup.setLastAssignmentConfigs(
+ value.assignmentConfigs().stream()
+ .collect(Collectors.toMap(
+ StreamsGroupMetadataValue.AssignmentConfig::key,
+ StreamsGroupMetadataValue.AssignmentConfig::value
+ ))
+ );
Review Comment:
The schema marks AssignmentConfigs as nullable with a default of null; for
older (pre-field) or null-valued records value.assignmentConfigs() will be
null, causing a NullPointerException when calling stream(). Guard with a
null/empty check (e.g., List<...> configs = value.assignmentConfigs(); if
(configs != null) { ... } else set empty) to ensure backward-compatible replay.
```suggestion
if (value.assignmentConfigs() != null) {
streamsGroup.setLastAssignmentConfigs(
value.assignmentConfigs().stream()
.collect(Collectors.toMap(
StreamsGroupMetadataValue.AssignmentConfig::key,
StreamsGroupMetadataValue.AssignmentConfig::value
))
);
} else {
streamsGroup.setLastAssignmentConfigs(Collections.emptyMap());
}
```
##########
group-coordinator/src/main/resources/common/message/StreamsGroupMetadataValue.json:
##########
@@ -26,6 +26,23 @@
{ "name": "MetadataHash", "versions": "0+", "type": "int64",
"about": "The hash of all topics in the group." },
{ "name": "ValidatedTopologyEpoch", "versions": "0+", "taggedVersions":
"0+", "tag": 0, "default": -1, "type": "int32",
- "about": "The topology epoch whose topics where validated to be present
in a valid configuration in the metadata." }
+ "about": "The topology epoch whose topics where validated to be present
in a valid configuration in the metadata." },
Review Comment:
Corrected spelling of 'where' to 'were'.
```suggestion
"about": "The topology epoch whose topics were validated to be present
in a valid configuration in the metadata." },
```
##########
group-coordinator/src/test/java/org/apache/kafka/coordinator/group/streams/StreamsGroupBuilder.java:
##########
@@ -74,6 +75,11 @@ public StreamsGroupBuilder withTargetAssignmentEpoch(int
targetAssignmentEpoch)
return this;
}
+ public StreamsGroupBuilder withLastAssignmentConfigs(Map<String, String>
lastAssignmentConfigs) {
+ this.lastAssignmentConfigs.putAll(lastAssignmentConfigs);
+ return this;
Review Comment:
[nitpick] withLastAssignmentConfigs does not validate the argument; passing
null will throw a NullPointerException on putAll. Add
Objects.requireNonNull(lastAssignmentConfigs, "lastAssignmentConfigs cannot be
null") or handle null explicitly for clearer test failures.
##########
group-coordinator/src/main/resources/common/message/StreamsGroupMetadataValue.json:
##########
@@ -26,6 +26,23 @@
{ "name": "MetadataHash", "versions": "0+", "type": "int64",
"about": "The hash of all topics in the group." },
{ "name": "ValidatedTopologyEpoch", "versions": "0+", "taggedVersions":
"0+", "tag": 0, "default": -1, "type": "int32",
- "about": "The topology epoch whose topics where validated to be present
in a valid configuration in the metadata." }
+ "about": "The topology epoch whose topics where validated to be present
in a valid configuration in the metadata." },
+ {
+ "name": "AssignmentConfigs",
+ "type": "[]AssignmentConfig",
+ "taggedVersions": "0+",
+ "nullableVersions": "0+",
+ "tag": 1,
+ "default": null,
Review Comment:
AssignmentConfigs is declared nullable with default null, but runtime replay
logic assumes non-null and directly streams over the list; either change
default to [] and remove nullableVersions (if always present post-upgrade) or
update replay code to handle null safely for backward compatibility.
```suggestion
"tag": 1,
"default": [],
```
--
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]
