fix(amber): report an uninitialized execution instead of NPEing, and map unparseable frames - #7802
Open
aglinxinyuan wants to merge 1 commit into
Open
Conversation
…map unparseable frames
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 353 | 0.215 | 29,694/34,152/34,152 us | 🔴 +23.7% / 🔴 +132.9% |
| 🔴 | bs=100 sw=10 sl=64 | 783 | 0.478 | 125,795/166,881/166,881 us | 🔴 +9.7% / 🔴 +56.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 893 | 0.545 | 1,117,591/1,197,216/1,197,216 us | ⚪ within ±5% / 🔴 +18.2% |
Baseline details
Latest main 875aa72 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 353 tuples/sec | 410 tuples/sec | 771.02 tuples/sec | -13.9% | -54.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.215 MB/s | 0.25 MB/s | 0.471 MB/s | -14.0% | -54.3% |
| bs=10 sw=10 sl=64 | p50 | 29,694 us | 24,003 us | 12,749 us | +23.7% | +132.9% |
| bs=10 sw=10 sl=64 | p95 | 34,152 us | 33,028 us | 15,594 us | +3.4% | +119.0% |
| bs=10 sw=10 sl=64 | p99 | 34,152 us | 33,028 us | 19,320 us | +3.4% | +76.8% |
| bs=100 sw=10 sl=64 | throughput | 783 tuples/sec | 813 tuples/sec | 997.05 tuples/sec | -3.7% | -21.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.478 MB/s | 0.496 MB/s | 0.609 MB/s | -3.6% | -21.5% |
| bs=100 sw=10 sl=64 | p50 | 125,795 us | 121,768 us | 100,339 us | +3.3% | +25.4% |
| bs=100 sw=10 sl=64 | p95 | 166,881 us | 152,064 us | 106,781 us | +9.7% | +56.3% |
| bs=100 sw=10 sl=64 | p99 | 166,881 us | 152,064 us | 113,101 us | +9.7% | +47.6% |
| bs=1000 sw=10 sl=64 | throughput | 893 tuples/sec | 919 tuples/sec | 1,037 tuples/sec | -2.8% | -13.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.545 MB/s | 0.561 MB/s | 0.633 MB/s | -2.9% | -13.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,117,591 us | 1,077,876 us | 971,388 us | +3.7% | +15.1% |
| bs=1000 sw=10 sl=64 | p95 | 1,197,216 us | 1,142,830 us | 1,013,249 us | +4.8% | +18.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,197,216 us | 1,142,830 us | 1,039,879 us | +4.8% | +15.1% |
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,566.72,200,128000,353,0.215,29693.77,34151.93,34151.93
1,100,10,64,20,2553.69,2000,1280000,783,0.478,125795.45,166881.02,166881.02
2,1000,10,64,20,22392.18,20000,12800000,893,0.545,1117591.01,1197216.02,1197216.02
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7802 +/- ##
============================================
- Coverage 91.34% 91.33% -0.01%
+ Complexity 4479 4477 -2
============================================
Files 1171 1171
Lines 47199 47200 +1
Branches 5303 5303
============================================
- Hits 43114 43112 -2
- Misses 2428 2429 +1
- Partials 1657 1659 +2
*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:
|
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 defects in
WorkflowWebsocketResource, all previously characterized without being cemented (#7303, #7676) so a fix would not have to fight a test asserting the broken behaviour.1 & 2. A workflow with no execution NPEs instead of reporting "not initialized".
WorkflowService.executionServiceis aBehaviorSubjectwith no initial value (WorkflowService.scala:140), sogetValueisnulluntil an execution is published.case other =>usedworkflowStateOpt.map(_.executionService.getValue), which wraps that null intoSome(null)— walking past thecase Nonearm that exists to report the friendly error, then NPEing onvalue.wsInput. Now uses the already-computedexecutionStateOpt, which is built withOption(...), soSome(null)cannot form and the existingNonearm actually fires.ModifyLogicRequestarm had the same gap in a different shape: its guard tested the workflow where it meant the execution. NowexecutionStateOpt.getOrElse(throw new IllegalStateException("workflow execution is not initialized")).Line 89 of this file already used
Option(...)correctly, as doesWorkflowServiceat its lines 208 and 349 — the fix adopts the established in-tree idiom rather than inventing one.A shape decision worth reviewing. For the
ModifyLogicRequestarm I did not simply swap the outer condition toexecutionStateOpt.isDefined. That variant makes a workflow-without-execution silently do nothing instead of reporting, which is not the intent — and it would collide with the pre-existing test "ignore a ModifyLogicRequest that arrives before any workflow is attached", which pins the no-workflow case asnoExceptionplussent shouldBe empty. Keeping the workflow guard and reporting the absent execution satisfies both.3. An unparseable frame no longer escapes the error mapper.
objectMapper.readValuemoved from above thetryto its first statement, so a frame the mapper cannot bind is reported like any handler failure.sessionStateandexecutionStateOptstay outside, because thecatcharm needs them — both routing arms still work.All messages use the existing wording,
"workflow execution is not initialized".The fixes are pinned
Four new tests. Verified in both directions, with the production file reverted and restored:
WorkflowWebsocketResourceSpecThe before-state failures are the right ones, from the JUnit XML (sbt's only reporter here is
-u, so the console shows no per-test lines):Expected java.lang.IllegalStateException … java.lang.NullPointerException was thrownModifyLogicRequestbefore any execution existsExpected java.lang.IllegalStateException … java.lang.NullPointerException was thrownList() was not equal to List("WorkflowErrorEvent")List() was not equal to List(COMPILATION_ERROR)None of the 15 pre-existing tests regressed.
Spec comments were updated, not just tests added
The spec's header paragraph on malformed frames, its "deliberately not covered" entry for
ModifyLogicRequest, and two in-test notes all documented these as known-and-unpinned. Leaving them would have left the spec asserting one thing and explaining the opposite, so they are rewritten to match.Trap avoidance, all previously encountered in this file: no assertions on
ClusterListener.numWorkerNodesInCluster(its default is0, so such an assertion passes even against a hard-coded literal);PrivilegeEnum.WRITEis fed rather than theNONEdefault; and the new tests useTestWorkflowService, which overridesdisconnect(), soafterEachnever reaches the nullAmberRuntime._actorSystem— the same pattern the existing tests use.Verification
WorkflowWebsocketResourceSpec: 19/19.TexeraWebSocketRequestSpec,SessionStateSpec,ServletAwareConfiguratorSpec,WebsocketInputSpec— 28/28 across 4 suites.TexeraWebSocketRequestSpecpinsInvalidTypeIdExceptionat the mapper level, which these changes leave untouched.scalafmtCheck,Test/scalafmtCheck,scalafixAll --checkall pass.Any related issues, documentation, discussions?
Closes #7801
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)