aglinxinyuan opened a new pull request, #7804:
URL: https://github.com/apache/texera/pull/7804
### What changes were proposed in this PR?
Three error paths that silently lost the information they exist to surface.
All were characterized during earlier coverage work and deliberately left
unpinned so a fix would not have to fight a test asserting the broken behaviour.
**1. `OPVersion.getVersion` no longer returns `null` when `git log` fails.**
The `GitAPIException` catch now stores the same `"N/A"` sentinel its
`NullPointerException` sibling already did, so the result is non-null *and*
memoized — previously the trailing `opMap.get(operatorName)` handed back
`null`, which propagated into operator metadata (`LogicalOp.scala:462-466` →
`OperatorMetadataGenerator.scala:171`), and every later call re-ran the failing
`git log`. `printStackTrace()` is kept.
That `"N/A"` is the intended sentinel is corroborated downstream:
`agent-service/src/agent/util/workflow-utils.ts:193` already defends with `??
"N/A"`.
**2. `InputManager.startInputPortReaderThreads` now passes the cause.** It
wrapped a start failure as `new RuntimeException(s"…: ${e.getMessage}")`
without `e`, discarding the original stack trace. The reader thread itself
already does this correctly (`InputPortMaterializationReaderThread.scala:147`).
**3. `PekkoActorRefMappingService.retrieveActorRef`'s catch no longer
re-reads the failing value.** It bound `e` but never used it, instead reading
`actorService.parent` again to name the parent in its warning — so a
**persistently** unreachable parent threw straight out of the handler meant to
contain it. The message now omits the parent and passes `e` as the log
throwable, matching house style (`WorkflowActor.scala:144`,
`SyncExecutionResource.scala:705`).
**Why not hoist the read instead:** capturing the parent ref before the
`try` was considered and rejected — the read *is* what throws, so hoisting it
would let the failure escape `retrieveActorRef` entirely rather than fixing
anything.
### All three are pinned, verified in both directions
The production files were reverted and restored to confirm each test
actually fails without its fix:
| Suite | production reverted | with fixes |
|---|---|---|
| `OPVersionSpec` | **9 passed, 2 failed** | **11 passed** |
| `InputManagerSpec` + `PekkoActorRefMappingServiceSpec` | **20 passed, 2
failed** | **22 passed** |
The before-state failures are the right ones:
- `null was not equal to "N/A"` (both new OPVersion cases)
- `null equaled null the original failure must be attached as the cause`
- `java.lang.IllegalStateException: parent is unreachable at …
PekkoActorRefMappingService.retrieveActorRef` — i.e. thrown *out of* the
handler, exactly the predicted failure mode
### On the tests
- **OPVersion**: two cases using the existing unborn-HEAD throwaway-repo
technique — the `"N/A"` fallback, and that it is *memoized* so a failing `git
log` is not retried.
- **InputManager**: a `NoOpReaderThread` subclass (no-op `run()`, so no
storage is touched) is started and joined, then installed into the private map
by reflection — the same technique as the existing
`OutputPortStorageWriterThreadSpec.installWriterThread`. `Thread.start()` on a
TERMINATED thread then throws `IllegalThreadStateException` deterministically,
and the test asserts both the wrapping message and that `getCause` is that
exception.
- **PekkoActorRefMappingService**: the new case sets the failure counter to
`Int.MaxValue` so *every* read throws, not just the first — the existing
single-failure test could not have caught this. It also asserts the id remains
askable afterwards, i.e. was not wrongly marked queried.
Three stale comments were updated rather than left contradicting the new
assertions: `OPVersionSpec`'s "Deliberately NOT asserted" paragraph about the
null, and the defect notes on the two amber specs.
### Verification
- `OPVersionSpec` 11/11; `InputManagerSpec` +
`PekkoActorRefMappingServiceSpec` + `InputPortMaterializationReaderThreadSpec`
32/32 (the reader-thread spec included as the nearest neighbour to the
`InputManager` change).
- Specs were filtered deliberately rather than running the whole
`WorkflowOperator` module, because `FileScanSourceOpExecSpec` currently aborts
on Windows over a leaked handle — that is being fixed separately in #7800.
- `scalafmtCheck`, `Test/scalafmtCheck` and `scalafixAll --check` pass for
both modules. One pre-existing scalafix *warning* in an untouched file
(`OutputManagerSpec.scala:59`, unused suppression) is unrelated.
### Any related issues, documentation, discussions?
Closes #7803
### How was this PR tested?
```
sbt "WorkflowOperator/testOnly
org.apache.texera.amber.operator.metadata.OPVersionSpec"
"WorkflowExecutionService/testOnly
org.apache.texera.amber.engine.architecture.messaginglayer.InputManagerSpec
org.apache.texera.amber.engine.architecture.common.PekkoActorRefMappingServiceSpec"
```
```
[info] Tests: succeeded 11, failed 0, canceled 0, ignored 0, pending 0
[info] Tests: succeeded 22, 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]
