Skip to content

chore(amber): remove the unused EmptyReplayLogger - #7452

Open
aglinxinyuan wants to merge 1 commit into
apache:mainfrom
aglinxinyuan:chore/remove-empty-replay-logger
Open

chore(amber): remove the unused EmptyReplayLogger#7452
aglinxinyuan wants to merge 1 commit into
apache:mainfrom
aglinxinyuan:chore/remove-empty-replay-logger

Conversation

@aglinxinyuan

@aglinxinyuan aglinxinyuan commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Deletes EmptyReplayLogger and its spec — a no-op ReplayLogger that the log-replay factory no longer reaches. Pure deletion, no behaviour change: −171 lines.

The null-object role moved up a layer to EmptyReplayLogManagerImpl, which implements the no-ops inline:

ReplayLogManager.scala:51  -> EmptyReplayLogManagerImpl   (inline no-ops, live)
ReplayLogManager.scala:109 -> new ReplayLoggerImpl()      (the only ReplayLogger built in main)
                              EmptyReplayLogger           (never instantiated)

No configuration selects a logger class by name, so there is no path that revives it.

Reviewer note: the abstract base ReplayLogger is not touched — ReplayLoggerImpl extends it and is unaffected. EmptyReplayLogger acquired unit tests during the 2026 coverage work, which is why it currently looks live; the spec covers this class and nothing else.

Any related issues, documentation, discussions?

Closes #7449

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 "WorkflowExecutionService/Test/compile" — success.

Verification, re-runnable by a reviewer:

git grep -n EmptyReplayLogger                          # only the two deleted files
git grep -n "extends ReplayLogger" -- amber/src/main   # ReplayLoggerImpl remains

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

Generated-by: Claude Code (Claude Opus 5)

Copilot AI lite review requested due to automatic review settings August 9, 2026 11:56

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Aug 9, 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.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main e03d971 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 376 0.229 27,068/37,552/37,552 us 🔴 +15.3% / 🔴 +133.0%
🔴 bs=100 sw=10 sl=64 777 0.474 128,531/158,524/158,524 us 🔴 +6.6% / 🔴 +42.5%
bs=1000 sw=10 sl=64 912 0.556 1,091,646/1,151,546/1,151,546 us ⚪ within ±5% / 🔴 +8.4%
Baseline details

Latest main e03d971 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 376 tuples/sec 398 tuples/sec 752.25 tuples/sec -5.5% -50.0%
bs=10 sw=10 sl=64 MB/s 0.229 MB/s 0.243 MB/s 0.459 MB/s -5.8% -50.1%
bs=10 sw=10 sl=64 p50 27,068 us 23,470 us 13,006 us +15.3% +108.1%
bs=10 sw=10 sl=64 p95 37,552 us 37,192 us 16,117 us +1.0% +133.0%
bs=10 sw=10 sl=64 p99 37,552 us 37,192 us 19,286 us +1.0% +94.7%
bs=100 sw=10 sl=64 throughput 777 tuples/sec 826 tuples/sec 953.66 tuples/sec -5.9% -18.5%
bs=100 sw=10 sl=64 MB/s 0.474 MB/s 0.504 MB/s 0.582 MB/s -6.0% -18.6%
bs=100 sw=10 sl=64 p50 128,531 us 120,577 us 105,130 us +6.6% +22.3%
bs=100 sw=10 sl=64 p95 158,524 us 152,161 us 111,226 us +4.2% +42.5%
bs=100 sw=10 sl=64 p99 158,524 us 152,161 us 121,065 us +4.2% +30.9%
bs=1000 sw=10 sl=64 throughput 912 tuples/sec 917 tuples/sec 984.25 tuples/sec -0.5% -7.3%
bs=1000 sw=10 sl=64 MB/s 0.556 MB/s 0.56 MB/s 0.601 MB/s -0.7% -7.4%
bs=1000 sw=10 sl=64 p50 1,091,646 us 1,085,417 us 1,024,295 us +0.6% +6.6%
bs=1000 sw=10 sl=64 p95 1,151,546 us 1,185,028 us 1,062,710 us -2.8% +8.4%
bs=1000 sw=10 sl=64 p99 1,151,546 us 1,185,028 us 1,095,400 us -2.8% +5.1%
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,532.11,200,128000,376,0.229,27068.39,37551.70,37551.70
1,100,10,64,20,2574.97,2000,1280000,777,0.474,128531.45,158524.01,158524.01
2,1000,10,64,20,21940.33,20000,12800000,912,0.556,1091646.25,1151545.56,1151545.56

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.32%. Comparing base (e03d971) to head (ac3a565).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7452      +/-   ##
============================================
- Coverage     85.32%   85.32%   -0.01%     
+ Complexity     4151     4149       -2     
============================================
  Files          1169     1168       -1     
  Lines         46740    46738       -2     
  Branches       5202     5202              
============================================
- Hits          39882    39880       -2     
  Misses         5148     5148              
  Partials       1710     1710              
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from e03d971
agent-service 85.50% <ø> (ø) Carriedforward from e03d971
amber 80.88% <ø> (-0.01%) ⬇️
computing-unit-managing-service 50.72% <ø> (ø) Carriedforward from e03d971
config-service 65.97% <ø> (ø) Carriedforward from e03d971
file-service 69.05% <ø> (ø) Carriedforward from e03d971
frontend 87.85% <ø> (ø) Carriedforward from e03d971
notebook-migration-service 78.89% <ø> (ø) Carriedforward from e03d971
pyamber 97.51% <ø> (ø) Carriedforward from e03d971
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from e03d971

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

@Yicong-Huang

Copy link
Copy Markdown
Contributor

I also think this looks unused. @shengquan-ni can you help confirm?

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 EmptyReplayLogger from the amber engine

4 participants