aglinxinyuan opened a new pull request, #7785:
URL: https://github.com/apache/texera/pull/7785
### What changes were proposed in this PR?
Removes six unreachable or unused regions in amber. **46 lines deleted, 2
inserted** across 7 files. Every claim was verified by grep across all file
types, including the Python side and `.proto` files where an RPC constant could
plausibly be mirrored.
| Removed | Evidence |
|---|---|
| `PekkoActorService.sendToSelfOnce`, `ask` | zero call sites; `ask`'s only
other hit is the `pekko.pattern.ask` inside its own body |
| `AsyncRPCClient.IgnoreReply = -1` | unreferenced; every other hit is the
distinct `IgnoreReplyAndDoNotLog = -2` |
| `ExecutionResultService`'s `case _ => throw` | `WebOutputMode` is `sealed
abstract` with exactly three `final case class` subtypes, all matched above |
| `UserQuotaResource`'s `executions == null` | jOOQ `fetch()` returns a
non-null `Result`; simplified to `isEmpty` |
| `ResultExportService`'s `zipStream == null` | `exportOperatorsAsZip`
either throws or returns a `new StreamingOutput { … }` literal |
| `WorkflowService.lastCompletedLogicalPlan` | write-only var: declaration,
one write, zero readers |
### Checks that changed the outcome
**Subclasses, not just call sites.** Three test classes extend
`PekkoActorService`, so a plain call-site grep would not have been enough.
Their overrides are `sendToSelfWithFixedDelay`
(`CoordinatorTimerServiceSpec.scala:420`) and `scheduleWithFixedDelay`
(`PekkoMessageTransferServiceSpec.scala:209`) — neither of the removed members.
**The Python side and the proto.** `IgnoreReply` could have been mirrored in
the Python worker or as a proto sentinel. It is not: `grep -rni
"ignore_reply|ignorereply|do_not_log"` over `amber/src/main/python` and
`src/test/python` returns nothing, and `commandId` is a plain `int64` field in
`controlcommands.proto` / `controlreturns.proto` with no named sentinel. No
literal `-1` is used as a commandId anywhere in `amber/src`.
**Exhaustivity was confirmed by the compiler, not by reading.** Removing the
`case _` arm produced **no** `match may not be exhaustive` warning, and the
total warning count is unchanged from the pre-edit baseline (8 test-compile
warnings before and after, all pre-existing and in unrelated files).
**The sibling guard is genuinely live.** `ResultExportService` has a second
`== null` guard on the adjacent path; `exportOperatorResultAsStream` really can
return null, and two `ResultExportServiceSpec` tests assert the error it
raises. That one is untouched.
### One judgement call worth reviewing
For `lastCompletedLogicalPlan` I removed the whole enclosing
`executionService.subscribe { … registerDiffHandler { … } }` block rather than
only the assignment. With the write gone the handler body reduces to
`Iterable.empty`, and `StateStore.registerDiffHandler` **appends** to
`diffHandlers` rather than replacing, so dropping a handler that emits nothing
is behaviour-preserving. It also disposes of a `Disposable` that was previously
being discarded. `WorkflowServiceSpec`'s own doc-comment described the var "and
the constructor's `executionService.subscribe` block that maintains it" as one
unit that "should be deleted instead", so this follows the documented intent —
but it is the one change here that is more than a strict deletion, so it
deserves a look.
Orphaned imports were removed with each deletion (`FutureBijection._`,
`COMPLETED`, `LogicalPlan`), since amber sets `-Ywarn-unused:imports`.
### No tests were removed
No spec exercised any deleted member. One stale 6-line "Deliberately not
covered" comment in `WorkflowServiceSpec` documented code that no longer
exists, so it is gone; the tests around it are untouched.
### Verification
- `WorkflowExecutionService/Test/compile` succeeds with the warning count
identical to baseline — no new exhaustivity or unused-import warnings.
- The 10 affected suites run green: **Suites completed 10, aborted 0; Tests
succeeded 153, failed 0.** Re-verified independently at 6 suites / 119 tests.
- `scalafmtCheck` (276 sources), `Test/scalafmtCheck` (202 sources) and
`scalafixAll --check` across the whole build all pass.
- Residual-reference greps for every removed symbol come back empty.
### Deliberately left alone
Two nearby findings are behavioural questions rather than cleanups, and are
**not** touched here:
- **`WorkerTimerService.resumeAdaptiveBatching` is a permanent no-op.**
`pauseAdaptiveBatching` has no call sites, and `isPaused` is only ever set
`true` inside it — so the live `resumeAdaptiveBatching` (called from
`ResumeHandler.scala:44`) resumes from a state nothing can enter. Whether pause
was meant to be wired into `PauseHandler` needs a maintainer's call.
- **`WorkflowExecution.getState`'s `READY` arm is unreachable**, because
`ExecutionUtils.aggregateStates` folds its all-ready case into `RUNNING` and
never returns `READY`. Whether that aggregation is intended is likewise not a
cleanup decision.
### Any related issues, documentation, discussions?
Closes #7784
### How was this PR tested?
```
STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly
org.apache.texera.amber.engine.architecture.common.PekkoActorRefMappingServiceSpec
org.apache.texera.amber.engine.common.rpc.AsyncRPCClientSpec
org.apache.texera.web.service.ExecutionResultServiceSpec
org.apache.texera.web.resource.dashboard.user.quota.UserQuotaResourceSpec
org.apache.texera.web.service.ResultExportServiceSpec
org.apache.texera.web.service.WorkflowServiceSpec"
```
```
[info] Suites: completed 6, aborted 0
[info] Tests: succeeded 119, failed 0, canceled 0, ignored 0, pending 0
```
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
