Skip to content

fix(amber): run completion cleanup - #8535

Open
anthonychengit wants to merge 1 commit into
apache:mainfrom
anthonychengit:fix/amber-completion-cleanup
Open

fix(amber): run completion cleanup#8535
anthonychengit wants to merge 1 commit into
apache:mainfrom
anthonychengit:fix/amber-completion-cleanup

Conversation

@anthonychengit

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Move workflow-completion timer cleanup to WorkflowExecutionManager, where terminal state is known, and run it under the existing one-shot completion guard.

Before: worker callback → wait for stats → region still terminating → cleanup missed
After:  all regions terminal → completion CAS → state update + timer cleanup once

The worker callback still starts the final statistics request, but no longer owns duplicate completion notification or cleanup.

Any related issues, documentation, discussions?

Closes #7032

How was this PR tested?

Regression coverage verifies cleanup runs exactly once for a completed workflow and never for an uninitialized workflow:

sbt "WorkflowExecutionService / Test / testOnly org.apache.texera.amber.engine.architecture.scheduling.WorkflowExecutionManagerSpec"
sbt scalafmtAll
sbt "scalafixAll --check"

The focused suite passes all 10 tests.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Codex was used for implementation and verification assistance. I reviewed the final code and test output before submission.

@Yicong-Huang Yicong-Huang added the release/v1.3 back porting to release/v1.3 label Sep 13, 2026
@github-actions
github-actions Bot requested a review from mengw15 September 13, 2026 19:57
@github-actions

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. A release/* label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required Backport Approvals check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.

Release branch Analysis
release/v1.3 Change detected on this branch — label added; this fix is queued to backport here. @mengw15 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other. Review requested.
⚠️ release/v1.2 Not labeled automatically — none of the files this PR modifies exist on this branch (amber/src/main/scala/org/apache/texera/amber/engine/architecture/coordinator/CoordinatorProcessor.scala, amber/src/main/scala/org/apache/texera/amber/engine/architecture/coordinator/promisehandlers/WorkerExecutionCompletedHandler.scala, amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/WorkflowExecutionManager.scala, amber/src/test/scala/org/apache/texera/amber/engine/architecture/scheduling/WorkflowExecutionManagerSpec.scala). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.2 by hand if this fix should be backported here — its release manager then has to approve before this PR can merge.

Auto-label run.

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @Yicong-Huang, @aglinxinyuan
    You can notify them by mentioning @Yicong-Huang, @aglinxinyuan in a comment.

@codecov-commenter

codecov-commenter commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.88%. Comparing base (7190a81) to head (0246f61).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8535      +/-   ##
============================================
- Coverage     95.88%   95.88%   -0.01%     
  Complexity      459      459              
============================================
  Files           588      588              
  Lines         33802    33766      -36     
  Branches       4217     4217              
============================================
- Hits          32411    32375      -36     
  Misses          813      813              
  Partials        578      578              
Flag Coverage Δ *Carryforward flag
access-control-service 80.18% <ø> (ø) Carriedforward from 7190a81
agent-service 99.32% <ø> (ø) Carriedforward from 7190a81
amber 90.49% <ø> (-0.34%) ⬇️
computing-unit-managing-service 77.14% <ø> (ø) Carriedforward from 7190a81
config-service 87.12% <ø> (ø) Carriedforward from 7190a81
file-service 83.65% <ø> (ø) Carriedforward from 7190a81
frontend 96.70% <ø> (ø) Carriedforward from 7190a81
notebook-migration-service 83.73% <ø> (ø) Carriedforward from 7190a81
pyamber 98.41% <ø> (ø) Carriedforward from 7190a81
workflow-compiling-service 74.09% <ø> (ø) Carriedforward from 7190a81

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 5 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 7190a81 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 745 0.455 12,886/17,532/17,532 us 🔴 +18.0% / 🔴 +19.3%
🔴 bs=100 sw=10 sl=64 1,756 1.072 55,041/89,035/89,035 us 🔴 +39.3% / 🟢 +60.4%
bs=1000 sw=10 sl=64 2,078 1.268 479,514/537,470/537,470 us ⚪ within ±5% / 🟢 +84.5%
Baseline details

Latest main 7190a81 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 745 tuples/sec 836 tuples/sec 838.23 tuples/sec -10.9% -11.1%
bs=10 sw=10 sl=64 MB/s 0.455 MB/s 0.51 MB/s 0.512 MB/s -10.8% -11.1%
bs=10 sw=10 sl=64 p50 12,886 us 10,920 us 11,878 us +18.0% +8.5%
bs=10 sw=10 sl=64 p95 17,532 us 19,281 us 14,690 us -9.1% +19.3%
bs=10 sw=10 sl=64 p99 17,532 us 19,281 us 18,483 us -9.1% -5.1%
bs=100 sw=10 sl=64 throughput 1,756 tuples/sec 1,837 tuples/sec 1,095 tuples/sec -4.4% +60.4%
bs=100 sw=10 sl=64 MB/s 1.072 MB/s 1.121 MB/s 0.668 MB/s -4.4% +60.4%
bs=100 sw=10 sl=64 p50 55,041 us 53,163 us 92,901 us +3.5% -40.8%
bs=100 sw=10 sl=64 p95 89,035 us 63,928 us 99,480 us +39.3% -10.5%
bs=100 sw=10 sl=64 p99 89,035 us 63,928 us 112,580 us +39.3% -20.9%
bs=1000 sw=10 sl=64 throughput 2,078 tuples/sec 2,105 tuples/sec 1,126 tuples/sec -1.3% +84.5%
bs=1000 sw=10 sl=64 MB/s 1.268 MB/s 1.285 MB/s 0.687 MB/s -1.3% +84.4%
bs=1000 sw=10 sl=64 p50 479,514 us 475,025 us 912,010 us +0.9% -47.4%
bs=1000 sw=10 sl=64 p95 537,470 us 533,424 us 950,922 us +0.8% -43.5%
bs=1000 sw=10 sl=64 p99 537,470 us 533,424 us 983,333 us +0.8% -45.3%
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,268.52,200,128000,745,0.455,12886.37,17532.23,17532.23
1,100,10,64,20,1139.08,2000,1280000,1756,1.072,55040.96,89035.02,89035.02
2,1000,10,64,20,9625.61,20000,12800000,2078,1.268,479513.75,537470.37,537470.37

@anthonychengit

Copy link
Copy Markdown
Contributor Author

/request-review

CI note: the remaining Amber failures are the repository-wide MinIO image outage (minio/minio:RELEASE.2025-02-28T09-55-16Z returns HTTP 404). The focused completion-cleanup suite passes as documented above.

@github-actions

Copy link
Copy Markdown
Contributor

Please specify at least one reviewer: /request-review @user.
Check the suggestion comment on this PR for candidates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine fix release/v1.3 back porting to release/v1.3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workflow-completion cleanup in WorkerExecutionCompletedHandler is unreachable when evaluated, so coordinator timers keep running

3 participants