Ashfaqbs commented on issue #1084: URL: https://github.com/apache/flink-agents/issues/1084#issuecomment-5769892938
You're right on all three, and I owe an acknowledgment: the occurrence-counter-inside-`generateKey()` design was a real regression from the lineage-based direction @wenjin272 and I had already converged on the day before — I lost the "identity must come from where an event sits in the causal tree, not when a call happens" property that made the lineage approach safe in the first place, and reintroduced exactly the order-dependence @wenjin272 had already ruled out once. Point 3 is the sharpest version of that: a completed sibling taking the `isCompleted()` fast-path during recovery is precisely the scenario that breaks any order-derived identity, counter or not. Concretely wrong in the counter proposal, matching your findings: - It assumed `generateKey()` is only called on the write path. It isn't — `get()` calls it too, on every lookup, so a miss-then-write pair silently burns two ordinals for one logical state, exactly as you traced through `processActionTaskForKey()`. - It assumed a new key segment was acceptable. `isKeyRetained()`'s hard-fail-on-five-segments behavior (and the existing test pinning that) makes a 5→6 change a recovery break for every already-persisted Kafka/Fluss key, which is the opposite of what durable execution is for. Reverting to the lineage-based design, with both of those folded in: - **Identity stays a fixed 5-segment key.** Instead of adding a segment, fold the lineage path into the *existing* event segment's derivation — `generateUUIDForEvent()` hashes `attributes` today; change it to hash `attributes + lineagePath`, where `lineagePath` is `(parent's own already-deterministic state key, this event's index in the parent's outputEvents list)`, recursively grounded at the root `InputEvent`'s `seqNum`. Old persisted keys stay parseable under the same five fields — they just correspond to a lineage path of "root, index 0" implicitly, so nothing about the recovery format changes. - **`generateKey()` stays pure.** The lineage path is computed once, at event-creation time in `RunnerContextImpl.sendEvent()` — where the parent action's `outputEvents` list and this event's position in it are both already available as plain, order-independent facts of that call — and carried on the `Event` itself (checkpointed with it), never recomputed later. `generateKey()` never mutates anything or depends on call order; it just hashes whatever lineage the event already carries, on both the `get()` and `put()` paths identically. - **This is why it survives recovery.** A sibling's lineage path is fixed by its parent's business logic (same inputs → same `outputEvents` → same index, every time), not by which sibling happens to finish its async wait first or which one takes the `isCompleted()` fast path. Two siblings racing during replay never share a mutable resource, so there's nothing left for the race to perturb. I'd still want to work through the open items @wenjin272 flagged before touching code: how an output's lineage-derived ordinal is preserved across an action-task's own yield/resume (my read: it's fixed at the moment the event is first created by `sendEvent()`, so a resumed continuation just carries the same `Event` object forward and never recomputes it — but I want to trace that against the actual yield path rather than assert it), and the determinism requirements for a re-executed-but-unfinished action, which is the same continuation-identity question from earlier in this thread. Given the community's noted 0.4-release capacity constraint, I'll hold off starting an implementation until there's a go-ahead, but wanted to close the loop on this critique specifically since it's correct and the open proposal on the issue right now (mine) doesn't yet reflect it. -- 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]
