aglinxinyuan commented on code in PR #4206:
URL: https://github.com/apache/texera/pull/4206#discussion_r3406971112
##########
amber/src/main/scala/org/apache/texera/web/service/WorkflowExecutionService.scala:
##########
@@ -66,6 +67,23 @@ class WorkflowExecutionService(
) extends SubscriptionManager
with LazyLogging {
+ // Loops require materialized edges to carry state between iterations.
+ // Previously we silently rewrote the user's execution mode to
+ // MATERIALIZED here, but that left the UI displaying the user's
+ // original (e.g. PIPELINED) choice while the engine ran something
+ // else -- the user had no way to tell the two had diverged. Fail
+ // loudly instead: surface a fatal error with an actionable message so
+ // the user can update the workflow setting and re-run.
+ if (
+ request.logicalPlan.operators.exists(_.isInstanceOf[LoopStartOpDesc])
Review Comment:
Re-applied in 7fd110a833 (it was lost in a force-rebase).
All three points:
1. **No longer tied to a class.** Added
`LogicalOp.requiresMaterializedExecution` (default false); `LoopStartOpDesc`
and `LoopEndOpDesc` both override it to true. `WorkflowExecutionService` checks
the flag, so the `LoopStartOpDesc` import/`isInstanceOf` is gone — any future
operator needing materialization just sets the flag.
2. **LoopEnd-only no longer slips through.** Because the check is
`operators.exists(_.requiresMaterializedExecution)` and LoopEnd sets the flag
too, a plan with a LoopEnd but no LoopStart now triggers the guard. There's an
explicit test for that case.
3. **Now tested.** Extracted the logic into a pure
`WorkflowExecutionService.validateExecutionMode(operators, settings)` (the
constructor delegates to it), and added `WorkflowExecutionServiceSpec`:
* `requiresMaterializedExecution` true for LoopStart/LoopEnd, false for a
non-loop op (SleepOpDesc);
* loop + PIPELINED → throws (incl. the LoopEnd-only plan);
* loop + MATERIALIZED, non-loop + PIPELINED, and an empty plan → no throw.
Verified locally (after migrating my local DB for the unrelated #5577 JOOQ
table): 6 new tests pass, the 29 LoopStart/LoopEnd op-desc specs pass, and
amber compiles. The behavior stays fail-loud (throw with an actionable
message), per the earlier decision on this thread.
--
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]
