eugenegujing opened a new pull request, #7648:
URL: https://github.com/apache/texera/pull/7648
### What changes were proposed in this PR?
This adds `AdvanceRegionExecutionsHandlerSpec`, the first direct coverage of
`AdvanceRegionExecutionsHandler` (added in #6960). The handler is four lines
long but carries three distinct contracts, none of which was asserted anywhere:
it delegates the advance to the workflow execution manager with the
coordinator's own actor service, it replies to the requesting round *without*
awaiting that advance (the whole point of routing the advance through its own
control round), and on failure it notifies the client with a `FatalError` that
carries the related worker id when the cause is a `WorkflowRuntimeException`
and `None` otherwise.
Six tests, test-only — no production file is touched:
1. `should advance the region executions with the coordinator's own actor
service` — asserts the forwarded `PekkoActorService` is the coordinator's own
instance (reference equality, plus a non-null guard so a vacuous `null == null`
cannot pass).
2. `should reply immediately instead of awaiting the advance` — the reply is
defined while the advance's promise is still unsatisfied. Deterministic, not
timing-based: the handler's `EmptyReturn()` becomes an already-satisfied future
through `returnAsFuture`, while the stubbed advance never completes. No sleeps
or polling anywhere in the spec.
3. `should reply to the requesting round while the advance is still running`
— delivers the request through `processDCM` and the real `AsyncRPCServer`, so
it also pins that the handler is dispatchable under
`METHOD_COORDINATOR_INITIATE_ADVANCE_REGION_EXECUTIONS`, the wire name
`PortCompletedHandler` actually sends.
4. `should notify the client of a failed advance with the related worker id`
5. `should notify the client of a failed advance without a worker id for
other failures`
6. `should not notify the client when the advance succeeds`
The fixture pattern is reused from the sibling handler specs (`TestKit` +
`RegionExecutionManagerTestSupport.createCoordinatorHarness()` + a real
`CoordinatorProcessor` and `CoordinatorAsyncRPCHandlerInitializer`, as in
`PortCompletedHandlerSpec` and `EmbeddedControlMessageHandlerSpec`). The only
new piece is a `WorkflowExecutionManager` subclass that records advance calls
and hands back a promise the test controls: driving the real manager into an
async failure requires staging a full region launch/termination, and the causes
it produces there are `IllegalStateException`s or the bare `Throwable` from
`ErrorUtils.reconstructThrowable` — never a `WorkflowRuntimeException` carrying
a worker id, which is the case test 4 exists to pin. Only the advance is
stubbed; the coordinator processor, RPC server dispatch, RPC client and output
gateway are all real.
Note on test 4: the `WorkflowRuntimeException` arm currently pins *intent*
rather than today-observable behavior. The two production subclasses that do
carry a worker id, `StateManager.InvalidStateException` and
`InvalidTransitionException` (`StateManager.scala:31-35`), are thrown
worker-side, and a failed worker RPC reaches the coordinator through
`ErrorUtils.reconstructThrowable`, which always rebuilds a plain `Throwable`
and so drops the type; every other production site constructs a
`WorkflowRuntimeException` with a message only, leaving `relatedWorkerId`
empty. Both arms would therefore behave identically at present. The branch is
in the handler and documented as intended, so the test guards it against
regression.
### Any related issues, documentation, discussions?
Closes #7647
The handler was introduced by #6960 (backported as #7096).
### How was this PR tested?
`sbt "WorkflowExecutionService/testOnly
*AdvanceRegionExecutionsHandlerSpec"` — 6 tests, all passing. No regressions in
the neighbourhood: `*coordinator.promisehandlers.* *scheduling.*` runs 20
suites / 166 tests / 0 failures. `WorkflowExecutionService/Test/scalafmtCheck`
is clean.
Each test was mutation-checked: the production handler was temporarily
broken, the suite re-run, and the handler reverted. Every behavior-changing
mutant is caught, and each test is the sole killer of at least one:
| Mutant | Caught by |
|---|---|
| `FatalError(err, err.relatedWorkerId)` → `FatalError(err, None)` | 4 |
| `FatalError(err, Some(COORDINATOR))` — wrong but non-empty worker id | 4 |
| drop the `.onFailure` block | 4, 5 |
| `.onFailure { case _ => () }` (swallow the failure) | 4, 5 |
| collapse both arms into a single catch-all | 4 |
| notify on `.onSuccess` instead | 4, 5, 6 |
| send the `FatalError` twice | 4 |
| await the advance (`.map(_ => EmptyReturn())`) | 2, 3 |
| never delegate (`Future.Unit` in place of the call) | 1, 2, 3, 4, 5 |
| `advanceRegionExecutions(null)` instead of `cp.actorService` | 1 |
| call `advanceRegionExecutions` twice | 3 |
The only surviving mutant, `EmptyReturn()` → `Future.value(EmptyReturn())`,
is semantically equivalent (`returnAsFuture` is an eager `Future.apply`), so
there is nothing to catch.
### 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
