joeyutong commented on code in PR #1185:
URL: https://github.com/apache/flink-agents/pull/1185#discussion_r4237219617
##########
api/src/main/java/org/apache/flink/agents/api/tools/ToolResponse.java:
##########
@@ -42,52 +59,59 @@ public class ToolResponse {
@JsonCreator
private ToolResponse(
- @JsonProperty("result") Object result,
+ @JsonProperty("blocks") List<? extends DataContentBlock> blocks,
+ @JsonProperty("metadata") Map<String, Object> metadata,
@JsonProperty("success") boolean success,
@JsonProperty("error") String error,
@JsonProperty("execution_time_ms") long executionTimeMs,
@JsonProperty("tool_name") String toolName) {
- this.result = result;
+ if (success != (error == null)) {
+ throw new IllegalArgumentException("ToolResponse success and error
disagree");
+ }
+ // Kryo restores collections by adding elements to the backing list.
+ this.blocks = blocks == null ? new ArrayList<>() : new
ArrayList<>(List.copyOf(blocks));
+ this.metadata = metadata == null ? Map.of() : metadata;
Review Comment:
`Map.of()` has a Flink-version compatibility caveat: **Flink 1.20.5 uses
Kryo 2.24.0**, which lacks dedicated serializers for JDK immutable maps. Its
generic map deserializer can call `put()` on an immutable map and fail. **Flink
2.3.0 uses Kryo 5.6.2**, which has dedicated immutable-map serializers.
I verified this with the existing `ReviewAnalysisAgent`: the non-empty
`Map.of("input", content)` fails with `UnsupportedOperationException` when
RocksDB reads the state on Flink 1.20.5, while the same case passes on Flink
2.3.0 + RocksDB. This is worth considering for `metadata` here as well when
supporting Flink 1.20.
--
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]
