Skip to content

chore(amber): remove unused OrderingEnforcer - #7028

Merged
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:remove-dead-amber-engine-code
Jul 30, 2026
Merged

chore(amber): remove unused OrderingEnforcer#7028
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:remove-dead-amber-engine-code

Conversation

@aglinxinyuan

@aglinxinyuan aglinxinyuan commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Deletes OrderingEnforcer, a generic helper in the amber engine with no production caller. Pure deletion, no behaviour change: −203 lines.

Removed Lines Why it is dead
architecture/messaginglayer/OrderingEnforcer.scala 53 last caller removed by #2208
architecture/messaginglayer/OrderingEnforcerSpec.scala 150 covers only the above

OrderingEnforcer was the extracted, generic form of the FIFO / exactly-once reorder buffer. #2208 ("Refactoring of amber engine", 2023-11-15) inlined that logic into AmberFIFOChannel, which still carries the identical header comment /* The abstracted FIFO/exactly-once logic */ and re-declares the same members:

OrderingEnforcer[T]  (deleted)        AmberFIFOChannel  (live, unchanged)
  ofoMap: LongMap[T]                    ofoMap: HashMap[Long, WorkflowFIFOMessage]
  current: Long                         current: Long
  isDuplicated / isAhead                isDuplicated / isAhead
  stash / enforceFIFO                   stash / enforceFIFO

The generic copy has been stranded for two and a half years.

Reviewer note: there is a separate, live OrderEnforcer trait (no -ing) in architecture/logreplay/, used by InputGateway/NetworkInputGateway. It is unrelated and untouched — only the messaginglayer OrderingEnforcer is removed here.

It acquired unit tests during the 2026 coverage work (#4721), which is why it currently looks live despite having been unreachable since 2023.

Scope change after review: this PR originally also removed Utils.retry. Per @Yicong-Huang's review it has been restored and is untouched here — Utils.scala and UtilsSpec.scala are byte-identical to main.

Any related issues, documentation, discussions?

Closes #7027

How was this PR tested?

Existing tests only — this PR adds none, since it removes code and the spec that covered it.

Locally, from the repo root with Java 17:

  • sbt "scalafixAll --check" — clean.
  • sbt scalafmtCheckAll — clean.
  • sbt "WorkflowExecutionService/testOnly *UtilsSpec" — 74 tests, all pass (Utils.retry and its three tests are back and green).

Verification that nothing references the removed class, re-runnable by a reviewer:

git grep -nw OrderingEnforcer      # only the two deleted files

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

Generated-by: Claude Code (Claude Opus 5)

@github-actions

github-actions Bot commented Jul 29, 2026

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.

@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.26%. Comparing base (a61702f) to head (88ac489).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7028      +/-   ##
============================================
+ Coverage     79.20%   79.26%   +0.05%     
+ Complexity     3786     3775      -11     
============================================
  Files          1160     1159       -1     
  Lines         46118    46110       -8     
  Branches       5114     5113       -1     
============================================
+ Hits          36529    36549      +20     
+ Misses         7969     7942      -27     
+ Partials       1620     1619       -1     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 3ff77d3
agent-service 76.76% <ø> (ø) Carriedforward from 3ff77d3
amber 72.58% <ø> (+0.15%) ⬆️
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from 3ff77d3
config-service 66.66% <ø> (ø) Carriedforward from 3ff77d3
file-service 67.21% <ø> (ø) Carriedforward from 3ff77d3
frontend 83.08% <ø> (ø) Carriedforward from 3ff77d3
notebook-migration-service 78.94% <ø> (ø) Carriedforward from 3ff77d3
pyamber 96.34% <ø> (ø) Carriedforward from 3ff77d3
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 3ff77d3

*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 Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

CI benchmark results are noisy; treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 328 0.2 28,641/41,432/41,432 us 🔴 +163.3% / 🔴 +162.7%
🔴 bs=100 sw=10 sl=64 773 0.472 130,308/162,294/162,294 us 🔴 +42.8% / 🔴 +51.2%
🔴 bs=1000 sw=10 sl=64 891 0.544 1,111,614/1,218,279/1,218,279 us 🔴 +12.5% / 🔴 +18.6%
Baseline details

Latest main a61702f from 2026-07-29T14:37:20.598Z

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 328 tuples/sec 750.01 tuples/sec 786.12 tuples/sec -56.3% -58.3%
bs=10 sw=10 sl=64 MB/s 0.2 MB/s 0.458 MB/s 0.48 MB/s -56.3% -58.3%
bs=10 sw=10 sl=64 p50 28,641 us 12,994 us 12,305 us +120.4% +132.8%
bs=10 sw=10 sl=64 p95 41,432 us 15,738 us 15,774 us +163.3% +162.7%
bs=10 sw=10 sl=64 p99 41,432 us 18,799 us 18,978 us +120.4% +118.3%
bs=100 sw=10 sl=64 throughput 773 tuples/sec 938.38 tuples/sec 999.71 tuples/sec -17.6% -22.7%
bs=100 sw=10 sl=64 MB/s 0.472 MB/s 0.573 MB/s 0.61 MB/s -17.6% -22.6%
bs=100 sw=10 sl=64 p50 130,308 us 106,490 us 100,616 us +22.4% +29.5%
bs=100 sw=10 sl=64 p95 162,294 us 113,622 us 107,356 us +42.8% +51.2%
bs=100 sw=10 sl=64 p99 162,294 us 123,806 us 113,255 us +31.1% +43.3%
bs=1000 sw=10 sl=64 throughput 891 tuples/sec 966.12 tuples/sec 1,031 tuples/sec -7.8% -13.6%
bs=1000 sw=10 sl=64 MB/s 0.544 MB/s 0.59 MB/s 0.63 MB/s -7.7% -13.6%
bs=1000 sw=10 sl=64 p50 1,111,614 us 1,035,855 us 980,328 us +7.3% +13.4%
bs=1000 sw=10 sl=64 p95 1,218,279 us 1,083,196 us 1,027,528 us +12.5% +18.6%
bs=1000 sw=10 sl=64 p99 1,218,279 us 1,097,913 us 1,054,298 us +11.0% +15.6%
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,610.05,200,128000,328,0.200,28641.07,41431.81,41431.81
1,100,10,64,20,2588.81,2000,1280000,773,0.472,130308.42,162294.41,162294.41
2,1000,10,64,20,22436.29,20000,12800000,891,0.544,1111614.35,1218279.48,1218279.48

@aglinxinyuan aglinxinyuan changed the title chore(amber): remove unused OrderingEnforcer and Utils.retry chore(amber): remove unused OrderingEnforcer Jul 30, 2026
@aglinxinyuan
aglinxinyuan enabled auto-merge July 30, 2026 00:03
@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Jul 30, 2026
Merged via the queue into apache:main with commit abdf440 Jul 30, 2026
25 checks passed
@aglinxinyuan
aglinxinyuan deleted the remove-dead-amber-engine-code branch July 30, 2026 00:39
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.

Remove the unused OrderingEnforcer from the amber engine

3 participants