You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "aglinxinyuan (via GitHub)" <gi...@apache.org> on 2026/08/15 02:01:24 UTC
[PR] refactor(amber): collapse the single-subclass ReplayLogger base into ReplayLoggerImpl [texera]
aglinxinyuan opened a new pull request, #7674:
URL: https://github.com/apache/texera/pull/7674
### 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(amber): collapse the single-subclass ReplayLogger hierarchy into one class [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #7674:
URL: https://github.com/apache/texera/pull/7674
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(amber): collapse the single-subclass ReplayLogger base into ReplayLoggerImpl [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7674:
URL: https://github.com/apache/texera/pull/7674#issuecomment-5300007968
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/7674?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 85.32%. Comparing base ([`e03d971`](https://app.codecov.io/gh/apache/texera/commit/e03d97108ddd47befc78a87f467f781dd50a56e6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a76889e`](https://app.codecov.io/gh/apache/texera/commit/a76889ef1a178f6b22092044e42b5c2caa6fd2f5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 105 commits behind head on main.
:white_check_mark: All tests successful. No failed tests found.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #7674 +/- ##
============================================
- Coverage 85.32% 85.32% -0.01%
+ Complexity 4151 4148 -3
============================================
Files 1169 1168 -1
Lines 46740 46737 -3
Branches 5202 5202
============================================
- Hits 39882 39879 -3
Misses 5148 5148
Partials 1710 1710
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.50% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `80.88% <ø> (-0.01%)` | :arrow_down: | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.72% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.97% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.05% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.85% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.89% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `97.51% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/7674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.31% <ø> (ø)` | | Carriedforward from [ac3a565](https://app.codecov.io/gh/apache/texera/commit/ac3a56560c9a45c3ba6f50ae14a169f1712d30aa?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/7674?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(amber): collapse the single-subclass ReplayLogger base into ReplayLoggerImpl [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7674:
URL: https://github.com/apache/texera/pull/7674#issuecomment-5299993307
<!-- texera-benchmarks-comment -->
## ⚠️ 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](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/31858080756)
| | 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% |
<details><summary>Baseline details</summary>
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% |
</details>
<details><summary>Raw CSV</summary>
```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
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(amber): collapse the single-subclass ReplayLogger base into ReplayLoggerImpl [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #7674:
URL: https://github.com/apache/texera/pull/7674#discussion_r3793686537
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/logreplay/ReplayLoggerImpl.scala:
##########
@@ -28,7 +28,7 @@ import org.apache.texera.amber.engine.common.ambermessage.WorkflowFIFOMessage
import scala.collection.mutable
-class ReplayLoggerImpl extends ReplayLogger {
+class ReplayLoggerImpl {
Review Comment:
Consider renaming this concrete class to `ReplayLogger` now that the abstraction is gone. Keeping the `Impl` suffix implies a corresponding interface, unlike the other interface-less classes in this package.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/logreplay/ReplayLoggerImpl.scala:
##########
@@ -46,7 +46,7 @@ class ReplayLoggerImpl extends ReplayLogger {
* @param channel The channel ID associated with the processing step.
Review Comment:
The Scaladoc name should match the actual `channelId` parameter.
```suggestion
* @param channelId The channel ID associated with the processing step.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(amber): collapse the single-subclass ReplayLogger base into ReplayLoggerImpl [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7674:
URL: https://github.com/apache/texera/pull/7674#issuecomment-5299964721
<!-- texera-reviewer-suggestion -->
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(amber): collapse the single-subclass ReplayLogger hierarchy into one class [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on code in PR #7674:
URL: https://github.com/apache/texera/pull/7674#discussion_r3794036686
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/logreplay/ReplayLoggerImpl.scala:
##########
@@ -28,7 +28,7 @@ import org.apache.texera.amber.engine.common.ambermessage.WorkflowFIFOMessage
import scala.collection.mutable
-class ReplayLoggerImpl extends ReplayLogger {
+class ReplayLoggerImpl {
Review Comment:
Done in d844b3b — it is now just `ReplayLogger`, in `ReplayLogger.scala`, the file the base vacated. You are right that the suffix was advertising an interface this PR deletes, and the other suffix-free classes in the package (`AsyncReplayLogWriter`, `ReplayOrderEnforcer`) set the precedent. `ReplayLogManagerImpl` is untouched — the `ReplayLogManager` trait beside it is real, so its suffix still means something.
The rename reaches one production call site (`ReplayLogManager.scala:109`) plus the constructions and test names in `LogreplayPrimitivesSpec`. `git grep -rn ReplayLoggerImpl` is now empty.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/logreplay/ReplayLoggerImpl.scala:
##########
@@ -46,7 +46,7 @@ class ReplayLoggerImpl extends ReplayLogger {
* @param channel The channel ID associated with the processing step.
Review Comment:
Applied in d844b3b. I also re-aligned the two neighbouring descriptions — the block was column-aligned on the 7-character `channel`, so `channelId` widened it by two.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(amber): collapse the single-subclass ReplayLogger hierarchy into one class [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on PR #7674:
URL: https://github.com/apache/texera/pull/7674#issuecomment-5312280142
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org