fix(amber, operator): keep error information on three failure paths - #7804
Merged
aglinxinyuan merged 1 commit intoAug 20, 2026
Merged
Conversation
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7804 +/- ##
============================================
- Coverage 91.34% 91.33% -0.01%
+ Complexity 4479 4476 -3
============================================
Files 1171 1171
Lines 47199 47201 +2
Branches 5303 5303
============================================
- Hits 43114 43113 -1
Misses 2428 2428
- Partials 1657 1660 +3
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 379 | 0.232 | 25,990/48,539/48,539 us | 🔴 +21.8% / 🔴 +211.3% |
| ⚪ | bs=100 sw=10 sl=64 | 857 | 0.523 | 116,616/133,069/133,069 us | ⚪ within ±5% / 🔴 +24.6% |
| ⚪ | bs=1000 sw=10 sl=64 | 963 | 0.587 | 1,037,457/1,097,329/1,097,329 us | ⚪ within ±5% / 🔴 +8.3% |
Baseline details
Latest main 875aa72 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 379 tuples/sec | 419 tuples/sec | 771.02 tuples/sec | -9.5% | -50.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.232 MB/s | 0.256 MB/s | 0.471 MB/s | -9.4% | -50.7% |
| bs=10 sw=10 sl=64 | p50 | 25,990 us | 22,046 us | 12,749 us | +17.9% | +103.9% |
| bs=10 sw=10 sl=64 | p95 | 48,539 us | 39,860 us | 15,594 us | +21.8% | +211.3% |
| bs=10 sw=10 sl=64 | p99 | 48,539 us | 39,860 us | 19,320 us | +21.8% | +151.2% |
| bs=100 sw=10 sl=64 | throughput | 857 tuples/sec | 877 tuples/sec | 997.05 tuples/sec | -2.3% | -14.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.523 MB/s | 0.535 MB/s | 0.609 MB/s | -2.2% | -14.1% |
| bs=100 sw=10 sl=64 | p50 | 116,616 us | 113,078 us | 100,339 us | +3.1% | +16.2% |
| bs=100 sw=10 sl=64 | p95 | 133,069 us | 139,629 us | 106,781 us | -4.7% | +24.6% |
| bs=100 sw=10 sl=64 | p99 | 133,069 us | 139,629 us | 113,101 us | -4.7% | +17.7% |
| bs=1000 sw=10 sl=64 | throughput | 963 tuples/sec | 963 tuples/sec | 1,037 tuples/sec | 0.0% | -7.2% |
| bs=1000 sw=10 sl=64 | MB/s | 0.587 MB/s | 0.588 MB/s | 0.633 MB/s | -0.2% | -7.3% |
| bs=1000 sw=10 sl=64 | p50 | 1,037,457 us | 1,038,925 us | 971,388 us | -0.1% | +6.8% |
| bs=1000 sw=10 sl=64 | p95 | 1,097,329 us | 1,083,990 us | 1,013,249 us | +1.2% | +8.3% |
| bs=1000 sw=10 sl=64 | p99 | 1,097,329 us | 1,083,990 us | 1,039,879 us | +1.2% | +5.5% |
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,527.29,200,128000,379,0.232,25990.30,48538.92,48538.92
1,100,10,64,20,2334.98,2000,1280000,857,0.523,116616.09,133069.29,133069.29
2,1000,10,64,20,20778.94,20000,12800000,963,0.587,1037456.62,1097329.45,1097329.45
Contributor
|
Backport PR opened: draft #7809 (#7809) to |
aglinxinyuan
added a commit
that referenced
this pull request
Aug 20, 2026
The cherry-pick of #7804 hit modify/delete on all three of its spec files: each already existed on `main` (added by #7018, #7219 and #5451, none of which were backported) but is absent here, so git could not apply a diff and the automation committed `main`'s whole file. Two of the three compile and pass against this branch unchanged; `PekkoActorRefMappingServiceSpec` does not, because it is written against `main` names this branch predates: - `virtualidentity.util.COORDINATOR` is `CONTROLLER` here, so the import did not resolve at all; - the scaladoc pointed at `PekkoMessageTransferServiceSpec`, a sibling that does not exist on this branch, leaving a dangling `[[link]]`. The assertions are untouched: every method the suite drives is identical on this branch, and the production hunks of #7804 applied cleanly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.getVersionno longer returnsnullwhengit logfails. TheGitAPIExceptioncatch now stores the same"N/A"sentinel itsNullPointerExceptionsibling already did, so the result is non-null and memoized — previously the trailingopMap.get(operatorName)handed backnull, which propagated into operator metadata (LogicalOp.scala:462-466→OperatorMetadataGenerator.scala:171), and every later call re-ran the failinggit log.printStackTrace()is kept.That
"N/A"is the intended sentinel is corroborated downstream:agent-service/src/agent/util/workflow-utils.ts:193already defends with?? "N/A".2.
InputManager.startInputPortReaderThreadsnow passes the cause. It wrapped a start failure asnew RuntimeException(s"…: ${e.getMessage}")withoute, 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 boundebut never used it, instead readingactorService.parentagain 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 passeseas the log throwable, matching house style (WorkflowActor.scala:144,SyncExecutionResource.scala:705).Why not hoist the read instead: capturing the parent ref before the
trywas considered and rejected — the read is what throws, so hoisting it would let the failure escaperetrieveActorRefentirely 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:
OPVersionSpecInputManagerSpec+PekkoActorRefMappingServiceSpecThe 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 causejava.lang.IllegalStateException: parent is unreachable at … PekkoActorRefMappingService.retrieveActorRef— i.e. thrown out of the handler, exactly the predicted failure modeOn the tests
"N/A"fallback, and that it is memoized so a failinggit logis not retried.NoOpReaderThreadsubclass (no-oprun(), so no storage is touched) is started and joined, then installed into the private map by reflection — the same technique as the existingOutputPortStorageWriterThreadSpec.installWriterThread.Thread.start()on a TERMINATED thread then throwsIllegalThreadStateExceptiondeterministically, and the test asserts both the wrapping message and thatgetCauseis that exception.Int.MaxValueso 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
OPVersionSpec11/11;InputManagerSpec+PekkoActorRefMappingServiceSpec+InputPortMaterializationReaderThreadSpec32/32 (the reader-thread spec included as the nearest neighbour to theInputManagerchange).WorkflowOperatormodule, becauseFileScanSourceOpExecSpeccurrently aborts on Windows over a leaked handle — that is being fixed separately in fix(test): stop the file-handle leak aborting the operator suite, seed the interval spec #7800.scalafmtCheck,Test/scalafmtCheckandscalafixAll --checkpass 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?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)