Skip to content

fix(amber): consolidate retry policies - #8534

Open
anthonychengit wants to merge 1 commit into
apache:mainfrom
anthonychengit:fix/retry-loop-policy
Open

fix(amber): consolidate retry policies#8534
anthonychengit wants to merge 1 commit into
apache:mainfrom
anthonychengit:fix/retry-loop-policy

Conversation

@anthonychengit

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Move the final two blocking retry loops onto RetryUtil.withBackoff while preserving their public contracts.

Caller Policy retained or chosen
PythonProxyClient 3 attempts, constant 200 ms waits, per-attempt client cleanup, WorkflowRuntimeException on failure
URLFetchUtil Optional result, exact retry budget, 200/400/800/1600 ms backoff

RetryUtil now supports a delay multiplier and caller-defined terminal exception factory.

Any related issues, documentation, discussions?

Closes #7124

How was this PR tested?

The new contract tests failed to compile before the utility/API changes. After the implementation, 17 utility, 8 URL fetcher, and 10 Python client tests pass:

sbt "Util / Test / testOnly org.apache.texera.common.util.RetryUtilSpec" "WorkflowOperator / Test / testOnly org.apache.texera.amber.operator.source.fetcher.URLFetchUtilSpec" "WorkflowExecutionService / Test / testOnly org.apache.texera.amber.engine.architecture.pythonworker.PythonProxyClientSpec"
sbt scalafmtAll
sbt "scalafixAll --check"

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 release/v1.3 back porting to release/v1.3 release/v1.2 back porting to release/v1.2 labels 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 Change detected on this branch — label added; this fix is queued to backport here. @xuang7 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other. Review requested.

Auto-label run.

@github-actions
github-actions Bot requested a review from xuang7 September 13, 2026 19:57
@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: @aglinxinyuan, @mengw15, @Yicong-Huang
    You can notify them by mentioning @aglinxinyuan, @mengw15, @Yicong-Huang 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.71%. Comparing base (7190a81) to head (e62ba42).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8534      +/-   ##
============================================
- Coverage     95.88%   95.71%   -0.17%     
- Complexity      459      506      +47     
============================================
  Files           588      588              
  Lines         33802    34330     +528     
  Branches       4217     4337     +120     
============================================
+ Hits          32411    32860     +449     
- Misses          813      869      +56     
- Partials        578      601      +23     
Flag Coverage Δ *Carryforward flag
access-control-service 83.22% <ø> (+3.04%) ⬆️
agent-service 99.32% <ø> (ø) Carriedforward from 7190a81
amber 90.88% <100.00%> (+0.06%) ⬆️
computing-unit-managing-service 76.22% <ø> (-0.93%) ⬇️
config-service 89.25% <ø> (+2.12%) ⬆️
file-service 83.65% <ø> (ø) Carriedforward from 7190a81
frontend 96.70% <ø> (ø) Carriedforward from 7190a81
notebook-migration-service 85.13% <ø> (+1.40%) ⬆️
pyamber 98.41% <ø> (ø) Carriedforward from 7190a81
workflow-compiling-service 73.93% <ø> (-0.17%) ⬇️

*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

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 0 better · 🔴 9 worse · ⚪ 6 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 379 0.231 25,987/35,637/35,637 us 🔴 +9.6% / 🔴 +142.6%
🔴 bs=100 sw=10 sl=64 782 0.477 124,200/157,910/157,910 us 🔴 +13.4% / 🔴 +58.7%
🔴 bs=1000 sw=10 sl=64 893 0.545 1,118,187/1,192,136/1,192,136 us 🔴 +5.4% / 🔴 +25.4%
Baseline details

Latest main 7190a81 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 379 tuples/sec 412 tuples/sec 838.23 tuples/sec -8.0% -54.8%
bs=10 sw=10 sl=64 MB/s 0.231 MB/s 0.251 MB/s 0.512 MB/s -8.0% -54.8%
bs=10 sw=10 sl=64 p50 25,987 us 23,710 us 11,878 us +9.6% +118.8%
bs=10 sw=10 sl=64 p95 35,637 us 33,733 us 14,690 us +5.6% +142.6%
bs=10 sw=10 sl=64 p99 35,637 us 33,733 us 18,483 us +5.6% +92.8%
bs=100 sw=10 sl=64 throughput 782 tuples/sec 814 tuples/sec 1,095 tuples/sec -3.9% -28.6%
bs=100 sw=10 sl=64 MB/s 0.477 MB/s 0.497 MB/s 0.668 MB/s -4.0% -28.6%
bs=100 sw=10 sl=64 p50 124,200 us 120,239 us 92,901 us +3.3% +33.7%
bs=100 sw=10 sl=64 p95 157,910 us 139,246 us 99,480 us +13.4% +58.7%
bs=100 sw=10 sl=64 p99 157,910 us 139,246 us 112,580 us +13.4% +40.3%
bs=1000 sw=10 sl=64 throughput 893 tuples/sec 908 tuples/sec 1,126 tuples/sec -1.7% -20.7%
bs=1000 sw=10 sl=64 MB/s 0.545 MB/s 0.554 MB/s 0.687 MB/s -1.6% -20.7%
bs=1000 sw=10 sl=64 p50 1,118,187 us 1,102,010 us 912,010 us +1.5% +22.6%
bs=1000 sw=10 sl=64 p95 1,192,136 us 1,131,314 us 950,922 us +5.4% +25.4%
bs=1000 sw=10 sl=64 p99 1,192,136 us 1,131,314 us 983,333 us +5.4% +21.2%
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,527.84,200,128000,379,0.231,25986.78,35637.46,35637.46
1,100,10,64,20,2557.66,2000,1280000,782,0.477,124200.44,157910.21,157910.21
2,1000,10,64,20,22399.90,20000,12800000,893,0.545,1118187.43,1192135.78,1192135.78

@anthonychengit

Copy link
Copy Markdown
Contributor Author

/request-review

CI note: the remaining Amber and file-service failures are the repository-wide MinIO image outage (minio/minio:RELEASE.2025-02-28T09-55-16Z returns HTTP 404). The focused retry-policy suites pass 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.

@mengw15 mengw15 removed release/v1.2 back porting to release/v1.2 release/v1.3 back porting to release/v1.3 labels Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two retry loops still hand-rolled: URLFetchUtil retries without delay, PythonProxyClient waits a constant delay

4 participants