Skip to content

refactor(amber): collapse the single-subclass ReplayLogger base into ReplayLoggerImpl - #7674

Open
aglinxinyuan wants to merge 2 commits into
apache:mainfrom
aglinxinyuan:refactor/collapse-replay-logger-base
Open

refactor(amber): collapse the single-subclass ReplayLogger base into ReplayLoggerImpl#7674
aglinxinyuan wants to merge 2 commits into
apache:mainfrom
aglinxinyuan:refactor/collapse-replay-logger-base

Conversation

@aglinxinyuan

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Collapses the ReplayLogger abstract base into its only subclass, ReplayLoggerImpl. Once #7452 removes EmptyReplayLogger, the base has a single subclass and is never named as a type anywhere:

ReplayLogger Subclasses Declared-type sites
on main 2 1 — EmptyReplayLoggerSpec.scala:125, itself deleted by #7452
after #7452 1 0
ReplayLogManager.scala:109  private val replayLogger = new ReplayLoggerImpl()  -> infers the concrete class
ReplayLoggerImpl.scala:31   class ReplayLoggerImpl extends ReplayLogger        -> the only subclass
ReplayLogger.scala:28       abstract class ReplayLogger                        -> nothing else refers to it

So the base declares three abstract methods that exactly one class implements and that no call site dispatches through. Dropping it removes extends ReplayLogger, one override keyword that no longer overrides anything, and ReplayLogger.scala itself. The three method bodies are untouched: −42 / +2 lines, no behaviour change.

ReplayLoggerImpl keeps its name. ReplayLogManagerImpl sits beside a genuine ReplayLogManager trait in the same package, so the ...Impl suffix stays consistent there.

Stacked on #7452, which needs to merge first — without it the base still has two subclasses. Until then the diff here carries both commits; the collapse is the second one, ReplayLogger.scala + 2 lines.

Any related issues, documentation, discussions?

Closes #7673

How was this PR tested?

Existing tests only — this is a structural change with no behaviour change, and LogreplayPrimitivesSpec already covers ReplayLoggerImpl by constructing it directly (new ReplayLoggerImpl()), so it pins all three methods across the refactor without modification.

Locally, from the repo root with Java 17:

  • sbt "WorkflowExecutionService/Test/compile" — success.
  • sbt "WorkflowExecutionService/testOnly *LogreplayPrimitivesSpec *EmptyReplayLogManagerImplSpec *ReplayLogGeneratorSpec" — all green.
  • sbt scalafmtCheckAll "scalafixAll --check" — clean.

Verification, re-runnable by a reviewer:

git grep -n ReplayLogger -- '*.scala' | grep -v ReplayLoggerImpl   # empty

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

Generated-by: Claude Code (Claude Opus 5)

…ReplayLoggerImpl

After EmptyReplayLogger is removed, the abstract base ReplayLogger has one
subclass and is never named as a type: ReplayLogManagerImpl builds it as
`new ReplayLoggerImpl()`, so the inferred type is the concrete class.

Drops `extends ReplayLogger` and the now-meaningless `override`, and deletes
ReplayLogger.scala. The three method bodies are untouched; no behaviour change.
Copilot AI lite review requested due to automatic review settings August 15, 2026 02:01

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

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
    You can notify them by mentioning @Yicong-Huang in a comment.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 3b19f76 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 353 0.216 27,479/39,045/39,045 us 🔴 +20.8% / 🔴 +138.9%
bs=100 sw=10 sl=64 781 0.477 126,177/143,343/143,343 us ⚪ within ±5% / 🔴 +34.6%
bs=1000 sw=10 sl=64 906 0.553 1,102,489/1,186,504/1,186,504 us ⚪ within ±5% / 🔴 +16.1%
Baseline details

Latest main 3b19f76 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 353 tuples/sec 369 tuples/sec 779.28 tuples/sec -4.3% -54.7%
bs=10 sw=10 sl=64 MB/s 0.216 MB/s 0.225 MB/s 0.476 MB/s -4.0% -54.6%
bs=10 sw=10 sl=64 p50 27,479 us 22,754 us 12,712 us +20.8% +116.2%
bs=10 sw=10 sl=64 p95 39,045 us 45,993 us 16,345 us -15.1% +138.9%
bs=10 sw=10 sl=64 p99 39,045 us 45,993 us 19,050 us -15.1% +105.0%
bs=100 sw=10 sl=64 throughput 781 tuples/sec 821 tuples/sec 1,017 tuples/sec -4.9% -23.2%
bs=100 sw=10 sl=64 MB/s 0.477 MB/s 0.501 MB/s 0.621 MB/s -4.8% -23.2%
bs=100 sw=10 sl=64 p50 126,177 us 120,241 us 100,048 us +4.9% +26.1%
bs=100 sw=10 sl=64 p95 143,343 us 150,326 us 106,477 us -4.6% +34.6%
bs=100 sw=10 sl=64 p99 143,343 us 150,326 us 114,739 us -4.6% +24.9%
bs=1000 sw=10 sl=64 throughput 906 tuples/sec 913 tuples/sec 1,048 tuples/sec -0.8% -13.5%
bs=1000 sw=10 sl=64 MB/s 0.553 MB/s 0.557 MB/s 0.639 MB/s -0.7% -13.5%
bs=1000 sw=10 sl=64 p50 1,102,489 us 1,090,997 us 976,350 us +1.1% +12.9%
bs=1000 sw=10 sl=64 p95 1,186,504 us 1,170,039 us 1,022,084 us +1.4% +16.1%
bs=1000 sw=10 sl=64 p99 1,186,504 us 1,170,039 us 1,053,520 us +1.4% +12.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,566.39,200,128000,353,0.216,27479.28,39044.65,39044.65
1,100,10,64,20,2561.32,2000,1280000,781,0.477,126177.04,143343.37,143343.37
2,1000,10,64,20,22066.15,20000,12800000,906,0.553,1102489.20,1186503.64,1186503.64

@codecov-commenter

codecov-commenter commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.47%. Comparing base (e03d971) to head (a76889e).
⚠️ Report is 105 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7674      +/-   ##
============================================
+ Coverage     85.32%   87.47%   +2.15%     
- Complexity     4151     4381     +230     
============================================
  Files          1169     1174       +5     
  Lines         46740    46980     +240     
  Branches       5202     5250      +48     
============================================
+ Hits          39882    41097    +1215     
+ Misses         5148     4090    -1058     
- Partials       1710     1793      +83     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from ac3a565
agent-service 85.50% <ø> (ø) Carriedforward from ac3a565
amber 86.78% <100.00%> (+5.89%) ⬆️
computing-unit-managing-service 50.72% <ø> (ø) Carriedforward from ac3a565
config-service 65.97% <ø> (ø) Carriedforward from ac3a565
file-service 69.05% <ø> (ø) Carriedforward from ac3a565
frontend 87.85% <ø> (ø) Carriedforward from ac3a565
notebook-migration-service 78.89% <ø> (ø) Carriedforward from ac3a565
pyamber 97.51% <ø> (ø) Carriedforward from ac3a565
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from ac3a565

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

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

Labels

engine refactor Refactor the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collapse the single-subclass ReplayLogger base into ReplayLoggerImpl

3 participants