refactor(amber): collapse the single-subclass ReplayLogger hierarchy into one class

[aglinxinyuan]

What changes were proposed in this PR?

Collapses the two-file ReplayLogger hierarchy into one concrete class. #7452 removed EmptyReplayLogger, which left the abstract base with a single subclass and no declared-type site anywhere:

ReplayLogger	Subclasses	Declared-type sites
before #7452	2	1 — EmptyReplayLoggerSpec.scala:125, itself deleted by #7452
on main today	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 declared three abstract methods that exactly one class implemented and that no call site dispatched through. Dropping it removes extends ReplayLogger, one override keyword that no longer overrides anything, and one of the two files. The three method bodies move across untouched — no behaviour change.

Naming. The surviving class takes the plain ReplayLogger name. With the base gone there is no abstraction left for an ...Impl suffix to distinguish it from, and the suffix would advertise an interface that no longer exists. ReplayLogManagerImpl is unaffected — it does sit beside a real ReplayLogManager trait in the same package.

              before                                      after

ReplayLogger.scala      abstract class ReplayLogger   ReplayLogger.scala   class ReplayLogger
ReplayLoggerImpl.scala  class ReplayLoggerImpl        (deleted)
                              extends ReplayLogger

The rename touches one production call site (ReplayLogManager.scala:109) and the new ReplayLoggerImpl() constructions plus test names in LogreplayPrimitivesSpec. It also fixes a stale @param channel in the Scaladoc, whose parameter has been called channelId all along.

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 the class by constructing it directly, so it pins all three methods across the refactor. Its only edits are the type name.

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 -rn ReplayLoggerImpl                         # empty — no Impl name survives
git grep -nw ReplayLogger -- '*.scala' | grep -v Spec # 2 hits: the class, and its one call site

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

Generated-by: Claude Code (Claude Opus 5)

[Copilot]

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

[github-actions]

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]

⚠️ Benchmark changes need a look

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

Compared against main 1011ff7 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	351	0.214	29,782/38,923/38,923 us	🔴 +25.8% / 🔴 +140.7%
🔴	bs=100 sw=10 sl=64	820	0.501	119,104/164,217/164,217 us	🔴 +9.2% / 🔴 +55.5%
⚪	bs=1000 sw=10 sl=64	939	0.573	1,061,431/1,137,708/1,137,708 us	⚪ within ±5% / 🔴 +12.4%

Baseline details

Latest main 1011ff7 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	351 tuples/sec	406 tuples/sec	784.16 tuples/sec	-13.5%	-55.2%
bs=10 sw=10 sl=64	MB/s	0.214 MB/s	0.248 MB/s	0.479 MB/s	-13.7%	-55.3%
bs=10 sw=10 sl=64	p50	29,782 us	23,675 us	12,626 us	+25.8%	+135.9%
bs=10 sw=10 sl=64	p95	38,923 us	39,242 us	16,169 us	-0.8%	+140.7%
bs=10 sw=10 sl=64	p99	38,923 us	39,242 us	18,986 us	-0.8%	+105.0%
bs=100 sw=10 sl=64	throughput	820 tuples/sec	838 tuples/sec	1,023 tuples/sec	-2.1%	-19.9%
bs=100 sw=10 sl=64	MB/s	0.501 MB/s	0.511 MB/s	0.625 MB/s	-2.0%	-19.8%
bs=100 sw=10 sl=64	p50	119,104 us	114,695 us	99,185 us	+3.8%	+20.1%
bs=100 sw=10 sl=64	p95	164,217 us	150,442 us	105,616 us	+9.2%	+55.5%
bs=100 sw=10 sl=64	p99	164,217 us	150,442 us	113,681 us	+9.2%	+44.5%
bs=1000 sw=10 sl=64	throughput	939 tuples/sec	944 tuples/sec	1,057 tuples/sec	-0.5%	-11.1%
bs=1000 sw=10 sl=64	MB/s	0.573 MB/s	0.576 MB/s	0.645 MB/s	-0.5%	-11.2%
bs=1000 sw=10 sl=64	p50	1,061,431 us	1,056,327 us	965,435 us	+0.5%	+9.9%
bs=1000 sw=10 sl=64	p95	1,137,708 us	1,120,087 us	1,012,135 us	+1.6%	+12.4%
bs=1000 sw=10 sl=64	p99	1,137,708 us	1,120,087 us	1,042,088 us	+1.6%	+9.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,570.00,200,128000,351,0.214,29782.37,38922.54,38922.54
1,100,10,64,20,2438.33,2000,1280000,820,0.501,119103.75,164216.62,164216.62
2,1000,10,64,20,21291.82,20000,12800000,939,0.573,1061430.68,1137708.26,1137708.26

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.95%. Comparing base (1011ff7) to head (d844b3b).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...r/engine/architecture/logreplay/ReplayLogger.scala	94.44%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7674      +/-   ##
============================================
+ Coverage     90.93%   90.95%   +0.01%     
- Complexity     4452     4455       +3     
============================================
  Files          1175     1174       -1     
  Lines         47140    47139       -1     
  Branches       5284     5284              
============================================
+ Hits          42869    42873       +4     
+ Misses         2581     2580       -1     
+ Partials       1690     1686       -4     

Flag	Coverage Δ		*Carryforward flag
access-control-service	81.00% <ø> (ø)		Carriedforward from 1011ff7
agent-service	98.62% <ø> (ø)		Carriedforward from 1011ff7
amber	87.42% <94.73%> (+0.02%)	⬆️
computing-unit-managing-service	73.67% <ø> (ø)		Carriedforward from 1011ff7
config-service	86.73% <ø> (ø)		Carriedforward from 1011ff7
file-service	68.90% <ø> (ø)		Carriedforward from 1011ff7
frontend	92.44% <ø> (ø)		Carriedforward from 1011ff7
notebook-migration-service	83.74% <ø> (ø)		Carriedforward from 1011ff7
pyamber	97.57% <ø> (ø)		Carriedforward from 1011ff7
workflow-compiling-service	77.19% <ø> (ø)		Carriedforward from 1011ff7

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

🟡 0 must-fix · 2 advisory · 1 polish — the hierarchy collapse is behaviorally safe, with two recommendations and one documentation touch-up.

Design & architecture (1)

• ReplayLoggerImpl.scala:31 — rename the remaining concrete type to ReplayLogger to complete the collapse (advisory, see inline)

Conventions (1)

• Description: remove the stale statement that #7452 still needs to merge, and rebase the stacked branch (advisory)

Polish: 1 quick touch-up (see inline comments).

[aglinxinyuan]

Thanks @Yicong-Huang — all three addressed, force-pushed as d844b3b.

Item	Status
Rename the concrete type to ReplayLogger	Done — ReplayLoggerImpl is gone, git grep -rn ReplayLoggerImpl is empty
Scaladoc @param channel → @param channelId	Done, with the @param block re-aligned
Stale #7452 note + rebase	Rebased onto main; git dropped the already-merged commit, so the branch is a single commit now, and the note is out of the description

The description is rewritten around the new naming rather than the old "keeps its name" argument, since your point stands: with the base deleted there is no abstraction for ...Impl to distinguish against.

Re-verified locally from the repo root on Java 17:

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