Skip to content

test(amber): cover RetrieveWorkflowStateHandler state-propagation dispatch#6310

Merged
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:test-retrieve-workflow-state-handler
Jul 10, 2026
Merged

test(amber): cover RetrieveWorkflowStateHandler state-propagation dispatch#6310
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:test-retrieve-workflow-state-handler

Conversation

@aglinxinyuan

@aglinxinyuan aglinxinyuan commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Adds a unit test for RetrieveWorkflowStateHandler.retrieveWorkflowState. Using a real CoordinatorProcessor with a capturing output handler and a CoordinatorAsyncRPCHandlerInitializer, it invokes the handler and asserts the dispatched PropagateEmbeddedControlMessage request — covering target-operator collection, request construction, dispatch, and response-transform registration, for both an empty and a populated physical plan.

Any related issues, documentation, discussions?

Closes #6307.

How was this PR tested?

Added a new RetrieveWorkflowStateHandlerSpec (2 cases), a plain-JVM AnyFlatSpec — no ActorSystem, DB, Python, or storage. Verified locally on Java 17 (WorkflowExecutionService/testOnly, plus scalafixAll --check clean). Test-only change — no production code is modified.

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

Generated-by: Claude Code (Opus 4.8 [1M context])

Copilot AI review requested due to automatic review settings July 10, 2026 04:36
@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:

  • No candidates found from git blame history.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new Amber unit test to exercise RetrieveWorkflowStateHandler.retrieveWorkflowState by building a real CoordinatorProcessor + CoordinatorAsyncRPCHandlerInitializer, invoking the handler, and asserting the dispatched PropagateEmbeddedControlMessageRequest for both an empty and a populated physical plan.

Changes:

  • Introduce RetrieveWorkflowStateHandlerSpec with a lightweight coordinator fixture that captures outbound WorkflowFIFOMessages.
  • Add assertions covering dispatched control-invocation shape (method name, request fields) for an empty plan.
  • Add assertions that a populated plan targets all operators.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

✅ No material benchmark regressions detected

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

Compared against main 1b42558 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 374 0.228 25,934/32,070/32,070 us ⚪ within ±5% / 🔴 +112.2%
🟢 bs=100 sw=10 sl=64 788 0.481 125,157/150,325/150,325 us 🟢 -9.6% / 🔴 +40.4%
bs=1000 sw=10 sl=64 890 0.543 1,118,214/1,262,756/1,262,756 us ⚪ within ±5% / 🔴 +22.7%
Baseline details

Latest main 1b42558 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 374 tuples/sec 374 tuples/sec 779.42 tuples/sec 0.0% -52.0%
bs=10 sw=10 sl=64 MB/s 0.228 MB/s 0.228 MB/s 0.476 MB/s 0.0% -52.1%
bs=10 sw=10 sl=64 p50 25,934 us 26,318 us 12,550 us -1.5% +106.6%
bs=10 sw=10 sl=64 p95 32,070 us 33,142 us 15,115 us -3.2% +112.2%
bs=10 sw=10 sl=64 p99 32,070 us 33,142 us 18,652 us -3.2% +71.9%
bs=100 sw=10 sl=64 throughput 788 tuples/sec 787 tuples/sec 1,000 tuples/sec +0.1% -21.2%
bs=100 sw=10 sl=64 MB/s 0.481 MB/s 0.48 MB/s 0.61 MB/s +0.2% -21.2%
bs=100 sw=10 sl=64 p50 125,157 us 122,974 us 99,872 us +1.8% +25.3%
bs=100 sw=10 sl=64 p95 150,325 us 166,364 us 107,073 us -9.6% +40.4%
bs=100 sw=10 sl=64 p99 150,325 us 166,364 us 114,377 us -9.6% +31.4%
bs=1000 sw=10 sl=64 throughput 890 tuples/sec 903 tuples/sec 1,020 tuples/sec -1.4% -12.7%
bs=1000 sw=10 sl=64 MB/s 0.543 MB/s 0.551 MB/s 0.622 MB/s -1.5% -12.8%
bs=1000 sw=10 sl=64 p50 1,118,214 us 1,104,054 us 987,499 us +1.3% +13.2%
bs=1000 sw=10 sl=64 p95 1,262,756 us 1,244,773 us 1,028,784 us +1.4% +22.7%
bs=1000 sw=10 sl=64 p99 1,262,756 us 1,244,773 us 1,057,136 us +1.4% +19.5%
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,535.44,200,128000,374,0.228,25933.91,32070.39,32070.39
1,100,10,64,20,2536.96,2000,1280000,788,0.481,125157.12,150325.46,150325.46
2,1000,10,64,20,22478.88,20000,12800000,890,0.543,1118214.39,1262755.77,1262755.77

@codecov-commenter

codecov-commenter commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.91%. Comparing base (737763b) to head (9cde8cd).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6310      +/-   ##
============================================
+ Coverage     60.88%   60.91%   +0.03%     
- Complexity     3372     3373       +1     
============================================
  Files          1136     1136              
  Lines         44275    44275              
  Branches       4853     4853              
============================================
+ Hits          26958    26972      +14     
+ Misses        15839    15825      -14     
  Partials       1478     1478              
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from fe173ff
agent-service 44.59% <ø> (ø) Carriedforward from fe173ff
amber 66.46% <ø> (+0.08%) ⬆️
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from fe173ff
config-service 52.30% <ø> (ø) Carriedforward from fe173ff
file-service 63.97% <ø> (ø) Carriedforward from fe173ff
frontend 52.75% <ø> (ø) Carriedforward from fe173ff
notebook-migration-service 78.94% <ø> (ø) Carriedforward from fe173ff
pyamber 91.18% <ø> (ø) Carriedforward from fe173ff
workflow-compiling-service 55.14% <ø> (ø) Carriedforward from fe173ff

*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.

@aglinxinyuan aglinxinyuan requested a review from mengw15 July 10, 2026 04:48

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aglinxinyuan aglinxinyuan added this pull request to the merge queue Jul 10, 2026
Merged via the queue into apache:main with commit 9f9f85b Jul 10, 2026
25 checks passed
@aglinxinyuan aglinxinyuan deleted the test-retrieve-workflow-state-handler branch July 10, 2026 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit test coverage for RetrieveWorkflowStateHandler

4 participants