fix(workflow-operator): disallow multiple links into a loop operator's input port - #7154
Open
aglinxinyuan wants to merge 1 commit into
Open
fix(workflow-operator): disallow multiple links into a loop operator's input port#7154aglinxinyuan wants to merge 1 commit into
aglinxinyuan wants to merge 1 commit into
Conversation
…s input port A Loop Start with two upstream operators feeding its single input port was accepted by the GUI but rejected at StartWorkflow with "expected exactly one reader URI, got 2" (discussion apache#6966). Nothing in the editor hinted that the plan was invalid until the run failed. InputPort already has a `disallowMultiLinks` flag that the frontend honors in two places -- the editor refuses to draw a second link into such a port (workflow-editor.component.ts) and workflow validation requires exactly one input (validation-workflow.service.ts, via WorkflowUtilService.inputPortToPortDescription which maps the backend flag onto the operator predicate). The loop operators simply never set it. Set it on the shared LoopOpDesc input port, so it applies to both Loop Start and Loop End: 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 (double `update`, double back-edge), and a Loop Start additionally needs a single reader for the scheduler to resolve its bookkeeping URIs from. No frontend change is needed. The scheduler's `require` stays as a defense-in-depth backstop for programmatically built plans.
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7154 +/- ##
============================================
- Coverage 79.58% 79.56% -0.03%
+ Complexity 3837 3830 -7
============================================
Files 1160 1160
Lines 46188 46188
Branches 5145 5145
============================================
- Hits 36761 36750 -11
- Misses 7795 7801 +6
- Partials 1632 1637 +5
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 482 | 0.294 | 19,607/31,851/31,851 us | 🔴 -11.7% / 🔴 +101.8% |
| 🔴 | bs=100 sw=10 sl=64 | 1,205 | 0.735 | 82,690/95,488/95,488 us | 🔴 +5.9% / 🟢 +24.0% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,412 | 0.862 | 708,136/816,625/816,625 us | 🔴 +10.7% / 🟢 +41.0% |
Baseline details
Latest main 91ed98d from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 482 tuples/sec | 546 tuples/sec | 771.62 tuples/sec | -11.7% | -37.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.294 MB/s | 0.333 MB/s | 0.471 MB/s | -11.7% | -37.6% |
| bs=10 sw=10 sl=64 | p50 | 19,607 us | 17,611 us | 12,626 us | +11.3% | +55.3% |
| bs=10 sw=10 sl=64 | p95 | 31,851 us | 29,347 us | 15,786 us | +8.5% | +101.8% |
| bs=10 sw=10 sl=64 | p99 | 31,851 us | 29,347 us | 19,344 us | +8.5% | +64.7% |
| bs=100 sw=10 sl=64 | throughput | 1,205 tuples/sec | 1,248 tuples/sec | 971.56 tuples/sec | -3.4% | +24.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.735 MB/s | 0.762 MB/s | 0.593 MB/s | -3.5% | +23.9% |
| bs=100 sw=10 sl=64 | p50 | 82,690 us | 78,118 us | 103,463 us | +5.9% | -20.1% |
| bs=100 sw=10 sl=64 | p95 | 95,488 us | 101,167 us | 110,296 us | -5.6% | -13.4% |
| bs=100 sw=10 sl=64 | p99 | 95,488 us | 101,167 us | 118,690 us | -5.6% | -19.5% |
| bs=1000 sw=10 sl=64 | throughput | 1,412 tuples/sec | 1,436 tuples/sec | 1,001 tuples/sec | -1.7% | +41.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.862 MB/s | 0.877 MB/s | 0.611 MB/s | -1.7% | +41.0% |
| bs=1000 sw=10 sl=64 | p50 | 708,136 us | 693,809 us | 1,008,988 us | +2.1% | -29.8% |
| bs=1000 sw=10 sl=64 | p95 | 816,625 us | 737,789 us | 1,055,260 us | +10.7% | -22.6% |
| bs=1000 sw=10 sl=64 | p99 | 816,625 us | 737,789 us | 1,074,689 us | +10.7% | -24.0% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,415.22,200,128000,482,0.294,19606.70,31851.31,31851.31
1,100,10,64,20,1660.01,2000,1280000,1205,0.735,82689.96,95487.88,95487.88
2,1000,10,64,20,14162.85,20000,12800000,1412,0.862,708136.25,816624.84,816624.84
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
A Loop Start whose single input port is fed by two upstream operators is accepted by the GUI, then rejected at
StartWorkflow:Nothing in the editor hints the plan is invalid until the run fails (discussion #6966). The restriction itself is intended — fan-in belongs in a
Unionbefore the loop — but it should be visible while building the workflow.InputPortalready has adisallowMultiLinksflag, and the frontend honors it in two places:workflow-editor.component.tsadditionalMetadata.inputPorts[i].disallowMultiLinks)validation-workflow.service.tsWorkflowUtilService.inputPortToPortDescriptionmappingdisallowMultiLinks→disallowMultiInputsThe loop operators simply never set it. This PR sets it on the shared
LoopOpDescinput port — one line, no frontend change.It applies to both Loop Start and Loop End, deliberately: 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 (double
update, double back-edge), and a Loop Start additionally needs a single reader for the scheduler to resolve its bookkeeping URIs from.The scheduler's
requirestays as a defense-in-depth backstop for programmatically built plans (the discussion's author buildsWorkflowContentdirectly, bypassing the GUI).Note: a saved workflow that already has two links into a loop operator will now be flagged invalid in the editor rather than failing at run time — which is the intent, since such a plan cannot execute today either.
Any related issues, documentation, discussions?
Closes #7155
Addresses the GUI half of discussion #6966.
How was this PR tested?
LoopStartOpDescSpecandLoopEndOpDescSpeceach gain a case pinninginputPorts.head.disallowMultiLinks shouldBe true(both fail before the change, 29/29 pass after).scalafmtCheckAll+scalafixAll --checkclean on Java 17.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)