da-daken commented on PR #926: URL: https://github.com/apache/flink-agents/pull/926#issuecomment-5214607704
Thanks @weiqingy @pltbkd for the new review — every point was helpful. I’ve removed the extra id parameter on the Python side. Whether Java and Python should use the same recovery identity is worth discussing in a follow-up issue, as @pltbkd noted in the first point. I’ve also aligned Python recovery behavior with Java: when a PENDING slot is read, we re-execute and recover in that same slot — that was my oversight, thanks for catching it. The durableExecuteAllAsync-related changes haven’t been pushed yet — sorry for the confusion. The catch-all semantics I mentioned earlier no longer apply. What I meant was that system/infrastructure exceptions should propagate so users can see them; in ToolCallAction, we can keep the same approach as the serial path — a system failure surfaces as tool-level failures for all affected tools. On Layer A serialization failures: I’d like to correct my earlier wording — we should return a failed Outcome without persisting the slot as failed; the slot stays PENDING. On JDK < 21, we also don’t persist the slot; we throw instead, so on recovery we run call/reconcile, which matches the intent. When a system error happens before the slot is persisted, running call/reconcile on recovery is also what ToolCallAction expects. Re @pltbkd’s review: `durableExecuteAllAsync `already has a real use case — execution timeout. From the framework’s perspective that’s a failure; we surface a timeout exception so callers can retry or compensate (which would land in another slot). That made the old toException() behavior — wrapping everything in RuntimeException — more painful, so I extended it to restore the original exception type after deserialization. On the two thread pools question: I agree with a single shared pool + a cap on how many threads one record/batch can occupy, with batch parallelism = cores. A second pool would use extra memory without real benefit over enlarging the shared pool; semantically, a dedicated pool for durableExecuteAllAsync isn’t necessary — one pool is enough. Because parallel tool execution is on by default, it can contend with other keys; I’ll call that out clearly in the ConfigOption docs so users notice the behavior change. On configuration: agreed — we can simplify to a single knob: parallelism = 1 (serial) vs parallelism > 1 (parallel). -- 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]
