Xiao-zhen-Liu commented on code in PR #7154:
URL: https://github.com/apache/texera/pull/7154#discussion_r3696861027
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/loop/LoopEndOpDescSpec.scala:
##########
@@ -49,6 +49,14 @@ class LoopEndOpDescSpec extends AnyFlatSpec with
LoopOpDescSpecMixin {
info.outputPorts should have length 1
}
+ it should "disallow more than one link into its input port" in {
+ // Each reader on an input port replays the loop state independently, so a
+ // second link would make this Loop End consume the same iteration twice
+ // (double `update`, double back-edge). Declaring it on the port stops the
+ // GUI from drawing the second link (discussion #6966).
+ desc().operatorInfo.inputPorts.head.disallowMultiLinks shouldBe true
+ }
+
Review Comment:
This is the same test as the one in `LoopStartOpDescSpec`, for a flag set
once on the shared base class. `LoopOpDescSpecMixin` exists for exactly this --
an `assertSingleInputLink(desc)` helper there, or a single test in the mixin,
would stop the two copies from drifting.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/loop/LoopOpDesc.scala:
##########
@@ -94,7 +94,14 @@ abstract class LoopOpDesc extends LogicalOp {
operatorName,
operatorDescription,
OperatorGroupConstants.CONTROL_GROUP,
- inputPorts = List(InputPort()),
+ // A loop operator takes exactly one link on its input port. Every reader
+ // on a materialized input port replays that port's states independently,
+ // so a second link would deliver the loop state twice per iteration; a
+ // Loop Start additionally needs a single reader for the scheduler to
+ // resolve its bookkeeping URIs from. Declaring it here is what makes the
+ // GUI refuse to draw the second link, instead of the plan being rejected
+ // only at StartWorkflow (discussion #6966).
+ inputPorts = List(InputPort(disallowMultiLinks = true)),
Review Comment:
The note in the description -- "a saved workflow that already has two links
into a loop operator will now be flagged invalid in the editor" -- doesn't
hold. Validation reads the flag off the *saved operator predicate*, not off the
schema: `validation-workflow.service.ts:328` checks
`operator.inputPorts[i].disallowMultiInputs`, and
`WorkflowUtilService.updateOperatorVersion` only rebuilds ports from the schema
when the saved port list is empty (`workflow-util.service.ts:207-226`). So a
loop operator that was saved before this change keeps `disallowMultiInputs:
false` and stays green, and the run still fails at `StartWorkflow`. Same for a
workflow whose JSON was assembled outside the GUI and then opened in it --
which is the discussion author's case, so that flow isn't covered either.
What the change does fix is drawing the link:
`workflow-editor.component.ts:1124` reads the dynamic schema, so both new and
already-saved loop operators refuse a *new* second link. That's the valuable
half. Worth correcting the note; if you want validation to cover existing
content too, `inputPortToPortDescription` would need to be re-applied on load
(or the validation would need to fall back to the schema flag) -- that's a
bigger change and arguably its own PR.
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/loop/LoopStartOpDescSpec.scala:
##########
@@ -46,6 +46,15 @@ class LoopStartOpDescSpec extends AnyFlatSpec with
LoopOpDescSpecMixin {
info.outputPorts should have length 1
}
+ it should "disallow more than one link into its input port" in {
+ // The runtime resolves the loop's bookkeeping URIs from this port's single
+ // reader (WorkflowExecutionManager requires exactly one storage pair), and
+ // every extra reader would replay the loop state again. Declaring it on
+ // the port is what stops the GUI from drawing a second link at all, rather
+ // than failing at StartWorkflow (discussion #6966).
+ desc().operatorInfo.inputPorts.head.disallowMultiLinks shouldBe true
+ }
+
Review Comment:
This asserts the literal that was just written one line away, so the only
way it fails is if someone deletes that line. Worth having, but the behavior
users actually hit lives in the frontend: a case in
`validation-workflow.service.spec.ts` with two enabled links into a loop
operator, asserting the operator comes back invalid, would test the guard
rather than the constant -- and it would have surfaced the saved-predicate gap
I noted on `LoopOpDesc.scala`.
--
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]
