Repository navigation
[api][runtime][test] Add @JsonCreator to built-in event subclasses for ActionStateSerde durable recovery - #869
Conversation
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for this fix @rosemarYuan. Overall looks good to me, I left some comments about the details.
Besides, I think we need to state in the "custom-event-subclasses" section of the documentation that users must declare a JsonCreator for their custom Event subclasses.
| } | ||
|
|
||
| @Test | ||
| public void testBuiltinChatToolAndContextEventsRoundTripThroughOutputEvents() throws Exception { |
There was a problem hiding this comment.
The test case name is overly verbose and its meaning is unclear. suggest to: testBuiltInEventSerDeRoundTrip
| originalState.addEvent( | ||
| new ContextRetrievalResponseEvent(requestId, "query text", List.of(doc))); | ||
|
|
||
| byte[] serialized = ActionStateSerde.serialize(originalState); |
There was a problem hiding this comment.
Currently, we inject the type information of Event during serialization by declaring mixins for the ObjectMapper.
// Add type information for polymorphic Event deserialization
mapper.addMixIn(Event.class, EventTypeInfoMixin.class);
mapper.addMixIn(InputEvent.class, EventTypeInfoMixin.class);
mapper.addMixIn(OutputEvent.class, EventTypeInfoMixin.class);
This allows the ObjectMapper to reconstruct the specific subclass of Event during deserialization. But I think that it’s sufficient to keep only mapper.addMixIn(Event.class, EventTypeInfoMixin.class); here.
There was a problem hiding this comment.
Thanks for you suggestion. Updated the PR based on your comments: simplified the test name, kept only the base Event mixin in ActionStateSerde, and added documentation for custom Java Event subclasses to explain the required Jackson creator annotations during durable recovery.
…r ActionStateSerde durable recovery
0b51c59 to
2603cd9
Compare
| public ChatResponseEvent( | ||
| @JsonProperty("id") UUID id, | ||
| @JsonProperty("attributes") Map<String, Object> attributes) { | ||
| super(id, EVENT_TYPE, attributes); |
There was a problem hiding this comment.
Could the JSON creator rehydrate typed nested attributes during replay?
With durable execution enabled, Jackson constructs ChatResponseEvent through this creator using the generic attributes map, so the nested response value stays a LinkedHashMap. Since replay dispatches the event as a concrete ChatResponseEvent, Java actions using the supported concrete signature can hit a ClassCastException when calling event.getResponse().
Would it make sense to normalize the typed nested attributes in the JSON creator, or before dispatch, so replayed built-in events match the live-path object shape? I think the same question applies to the other event creators added for typed nested attributes.
There was a problem hiding this comment.
Thanks for the review suggestion. I agree this should be normalized before replay dispatch.
I did it in the JSON creator rather than in dispatch: the (UUID, Map) constructor is the Jackson recovery entry point for each concrete built-in event, so normalizing there makes the recovered object match the live-path shape immediately, without the runtime needing to know how to hydrate arbitrary event subclasses.
For each built-in event with typed nested attributes (ChatRequestEvent, ChatResponseEvent, ToolResponseEvent, ContextRetrievalResponseEvent), I moved its fromEvent() conversion into a per-event normalizeAttributes helper that the creator calls. Both paths now produce identical typed attributes, so a concrete-signature action calling e.g. getResponse() on a replayed event no longer hits a ClassCastException.
There was a problem hiding this comment.
We need to update the user notifications in the documentation accordingly.
There was a problem hiding this comment.
The custom-event hint in workflow_agent.md already notes this.
If the subclass stores typed objects in attributes, convert them back to their typed forms inside this constructor too, as the built-in events do.
Based on this, I added a code example under that hint showing how the built-in events do it, using ChatResponseEvent's creator and its normalizeAttributes helper.
| // directly and call typed getters WITHOUT going through fromEvent(). The JSON creator must | ||
| // therefore rehydrate nested attributes; otherwise these getters throw ClassCastException | ||
| // on the raw LinkedHashMap. Assert the typed getters directly on the deserialized | ||
| // instances. |
There was a problem hiding this comment.
This comment is overly verbose, and the phrase "going through fromEvent" describes a previous implementation that I believe should not appear in the final comment.
…in event JSON creators for replay
c5c39a6 to
99e4e5a
Compare
…r ActionStateSerde durable recovery (#869) * [api][runtime][test] Add @jsoncreator to built-in event subclasses for ActionStateSerde durable recovery * [api][runtime][test][doc] Normalize typed nested attributes in built-in event JSON creators for replay (cherry picked from commit c7a2971)
|
Backport to release-0.3 |
Linked issue: #868
Purpose of change
ActionStateSerdeuses Jackson polymorphic deserialization (@JsonTypeInfo(Id.CLASS)) to restore the concreteEventsubclasses persisted inActionState(taskEvent/outputEvents) for durable execution recovery. Jackson requires each concrete subclass to declare its own creator — the@JsonCreatoron the baseEventconstructor is not inherited.Only
InputEvent/OutputEventannotate their(UUID, Map<String, Object>)constructors with@JsonCreator/@JsonProperty. The six built-in event subclasses underorg.apache.flink.agents.api.event.*(ChatRequestEvent,ChatResponseEvent,ToolRequestEvent,ToolResponseEvent,ContextRetrievalRequestEvent,ContextRetrievalResponseEvent) have the same constructors but lack the annotations, so deserializing any of them from a persistedActionStatefails with:This PR annotates the
(UUID, Map<String, Object>)constructor of all six classes, consistent withInputEvent/OutputEvent.Tests
Added
ActionStateSerdeTest#testBuiltinChatToolAndContextEventsRoundTripThroughOutputEvents, which:ActionStateSerdeboth astaskEventand insideoutputEvents, asserting the concrete class is restored (fails with the exception above without the fix);fromEvent()consumption path used by the built-in actions (ChatMessage,ToolResponse,Document, andUUIDrequest ids), not raw maps.Verified with
mvn -pl runtime test -Dtest=ActionStateSerdeTest(11/11 pass) and the fullapimodule test suite.API
No signature changes. Adds Jackson annotations to existing public constructors of the six built-in event classes; serialized JSON format is unchanged.
Documentation
doc-neededdoc-not-neededdoc-included