wenjin272 commented on code in PR #756: URL: https://github.com/apache/flink-agents/pull/756#discussion_r3392856213
########## python/flink_agents/api/events/event_type.py: ########## @@ -0,0 +1,66 @@ +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################# +"""Built-in event-type constants, sourced from each ``XxxEvent.EVENT_TYPE``.""" + +from __future__ import annotations + +from flink_agents.api.events.chat_event import ( + ChatRequestEvent as _ChatRequestEvent, +) +from flink_agents.api.events.chat_event import ( + ChatResponseEvent as _ChatResponseEvent, +) +from flink_agents.api.events.context_retrieval_event import ( + ContextRetrievalRequestEvent as _ContextRetrievalRequestEvent, +) +from flink_agents.api.events.context_retrieval_event import ( + ContextRetrievalResponseEvent as _ContextRetrievalResponseEvent, +) +from flink_agents.api.events.event import ( + InputEvent as _InputEvent, +) +from flink_agents.api.events.event import ( + OutputEvent as _OutputEvent, +) +from flink_agents.api.events.tool_event import ( + ToolRequestEvent as _ToolRequestEvent, +) +from flink_agents.api.events.tool_event import ( + ToolResponseEvent as _ToolResponseEvent, +) + + +class EventType: + """Namespace of built-in event-type constants. + + Usage: ``@action(EventType.InputEvent)``. + """ + + InputEvent: str = _InputEvent.EVENT_TYPE + OutputEvent: str = _OutputEvent.EVENT_TYPE + ChatRequestEvent: str = _ChatRequestEvent.EVENT_TYPE + ChatResponseEvent: str = _ChatResponseEvent.EVENT_TYPE + ToolRequestEvent: str = _ToolRequestEvent.EVENT_TYPE + ToolResponseEvent: str = _ToolResponseEvent.EVENT_TYPE + ContextRetrievalRequestEvent: str = _ContextRetrievalRequestEvent.EVENT_TYPE + ContextRetrievalResponseEvent: str = _ContextRetrievalResponseEvent.EVENT_TYPE + + def __init__(self) -> None: Review Comment: I think the __init__ method is unnecessary. We don’t have this design in `ResourceName` either. ########## api/src/main/java/org/apache/flink/agents/api/annotation/Action.java: ########## @@ -59,12 +60,8 @@ @Target(ElementType.METHOD) @Retention(RetentionPolicy.RUNTIME) public @interface Action { - /** - * List of event type strings that this action should respond to. - * - * @return Array of event type strings - */ - String[] listenEventTypes(); + /** Event type name strings; multiple entries have OR semantics. */ + String[] value(); Review Comment: Here, we use `value` as the parameter name, whereas Python’s `Action` decorator uses `trigger_conditions`. I understand this discrepancy arises because in Java annotations, only a member named `value` can be used without explicitly specifying the parameter name during declaration. Therefore, to improve usability for users, I believe this inconsistency is acceptable. However, we should probably explain this in the comments. ########## plan/src/main/java/org/apache/flink/agents/plan/serializer/ActionJsonDeserializer.java: ########## @@ -66,10 +66,15 @@ public Action deserialize(JsonParser jsonParser, DeserializationContext deserial throw new IOException("Unsupported function type: " + funcType); } - // Deserialize listenEventTypes - List<String> listenEventTypes = new ArrayList<>(); - node.get("listen_event_types") - .forEach(eventTypeNode -> listenEventTypes.add(eventTypeNode.asText())); + // Deserialize trigger_conditions (fall back to legacy listen_event_types for older JSONs) + List<String> triggerConditions = new ArrayList<>(); + JsonNode triggerNode = node.get("trigger_conditions"); + if (triggerNode == null) { + triggerNode = node.get("listen_event_types"); Review Comment: Although both @weiqingy and @wzhero1 agree on handling backward compatibility, I believe it is unnecessary at this stage. Flink-Agents is still in the 0.x series, where API compatibility is not guaranteed. In fact, we have already introduced some breaking API changes in version 0.3 (e.g., #631), so maintaining backward compatibility specifically for listen_event_types offers limited value. We will begin preparing for API stability and compatibility commitments starting with the next release (0.4), and formally commit to backward compatibility from version 1.0 onwards. Until then, I consider keeping the codebase clean and simple to be a higher priority. -- 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]
