weiqingy commented on code in PR #756:
URL: https://github.com/apache/flink-agents/pull/756#discussion_r3366740037
##########
plan/src/main/java/org/apache/flink/agents/plan/AgentPlan.java:
##########
@@ -184,27 +184,27 @@ private void readObject(ObjectInputStream in) throws
IOException, ClassNotFoundE
private void extractActions(
String actionName,
- String[] listenEventTypeStrings,
+ String[] triggerEntries,
org.apache.flink.agents.plan.Function function,
Map<String, Object> config)
throws Exception {
- List<String> eventTypeNames = new
ArrayList<>(Arrays.asList(listenEventTypeStrings));
+ List<String> triggerConditions = new
ArrayList<>(Arrays.asList(triggerEntries));
- if (eventTypeNames.isEmpty()) {
+ if (triggerConditions.isEmpty()) {
throw new IllegalArgumentException(
"Action "
+ actionName
- + " must specify at least one event type via
listenEventTypes.");
+ + " must specify at least one trigger entry via
@Action(value = ...).");
}
// Create an Action
- Action action = new Action(actionName, function, eventTypeNames,
config);
+ Action action = new Action(actionName, function, triggerConditions,
config);
// Add to actions map
actions.put(action.getName(), action);
// Add to actionsByEvent map
- for (String eventTypeName : eventTypeNames) {
+ for (String eventTypeName : triggerConditions) {
Review Comment:
This site builds the routing map from the raw `triggerConditions`, but the
built-in path just below (`addBuiltAction`, L217) builds it through
`action.getListenEventTypes()` — and that accessor's javadoc says a follow-up
overrides it to "filter out non-type entries" once CEL lands. Python's
`agent_plan.py:148` also reads the raw field. So of the three routing sites,
only the built-in one — which never carries CEL — goes through the filtering
seam; the two that handle user-supplied conditions bypass it.
Today they're equivalent, since every entry is a plain event-type name. But
once CEL expressions enter `trigger_conditions`, these two sites would register
strings like `"type == EventType.ChatResponseEvent && retryCount > 0"` as
literal keys in `actionsByEvent` — exactly what the accessor exists to strip.
Would it make sense to route all three through the accessor now, or to defer
the accessor until the CEL PR and read the raw field everywhere in the
meantime? Either way the three sites agreeing seems worth locking in. Not a
blocker for this PR — flagging while the context is fresh.
--
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]
