fix(amber): correct WorkerSpec input port to stop flaky timeouts#6037
fix(amber): correct WorkerSpec input port to stop flaky timeouts#6037Ma77Ball wants to merge 7 commits into
Conversation
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 410 | 0.25 | 24,626/31,626/31,626 us | 🔴 +6.8% / 🔴 +113.8% |
| 🟢 | bs=100 sw=10 sl=64 | 826 | 0.504 | 120,297/132,654/132,654 us | 🟢 -5.7% / 🔴 +28.6% |
| ⚪ | bs=1000 sw=10 sl=64 | 947 | 0.578 | 1,059,108/1,087,570/1,087,570 us | ⚪ within ±5% / 🔴 -11.7% |
Baseline details
Latest main 9f9f85b from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 410 tuples/sec | 436 tuples/sec | 814.14 tuples/sec | -6.0% | -49.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.25 MB/s | 0.266 MB/s | 0.497 MB/s | -6.0% | -49.7% |
| bs=10 sw=10 sl=64 | p50 | 24,626 us | 23,065 us | 12,120 us | +6.8% | +103.2% |
| bs=10 sw=10 sl=64 | p95 | 31,626 us | 31,805 us | 14,792 us | -0.6% | +113.8% |
| bs=10 sw=10 sl=64 | p99 | 31,626 us | 31,805 us | 18,232 us | -0.6% | +73.5% |
| bs=100 sw=10 sl=64 | throughput | 826 tuples/sec | 845 tuples/sec | 1,050 tuples/sec | -2.2% | -21.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.504 MB/s | 0.516 MB/s | 0.641 MB/s | -2.3% | -21.3% |
| bs=100 sw=10 sl=64 | p50 | 120,297 us | 115,071 us | 96,227 us | +4.5% | +25.0% |
| bs=100 sw=10 sl=64 | p95 | 132,654 us | 140,626 us | 103,174 us | -5.7% | +28.6% |
| bs=100 sw=10 sl=64 | p99 | 132,654 us | 140,626 us | 111,033 us | -5.7% | +19.5% |
| bs=1000 sw=10 sl=64 | throughput | 947 tuples/sec | 940 tuples/sec | 1,072 tuples/sec | +0.7% | -11.7% |
| bs=1000 sw=10 sl=64 | MB/s | 0.578 MB/s | 0.574 MB/s | 0.655 MB/s | +0.7% | -11.7% |
| bs=1000 sw=10 sl=64 | p50 | 1,059,108 us | 1,062,279 us | 950,645 us | -0.3% | +11.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,087,570 us | 1,108,116 us | 991,936 us | -1.9% | +9.6% |
| bs=1000 sw=10 sl=64 | p99 | 1,087,570 us | 1,108,116 us | 1,022,498 us | -1.9% | +6.4% |
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,487.87,200,128000,410,0.250,24626.13,31626.01,31626.01
1,100,10,64,20,2420.20,2000,1280000,826,0.504,120296.91,132653.83,132653.83
2,1000,10,64,20,21111.53,20000,12800000,947,0.578,1059107.93,1087569.71,1087569.71
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6037 +/- ##
=========================================
Coverage 61.05% 61.05%
+ Complexity 3380 3378 -2
=========================================
Files 1140 1140
Lines 44932 44932
Branches 4944 4944
=========================================
+ Hits 27432 27433 +1
+ Misses 15987 15981 -6
- Partials 1513 1518 +5
*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:
|
Updated error handling comment for clarity. Signed-off-by: Matthew B. <mgball@uci.edu>
| // Re-throw Errors (e.g. failed assertions) after replying; only | ||
| // Exceptions are returned to the sender and recovered from. | ||
| if (err.isInstanceOf[Error]) throw err |
There was a problem hiding this comment.
how come non errors can reach here? that's a bigger problem...
with your change, this "non-error" is omitted silently...
can we at least log warning when it happens?
There was a problem hiding this comment.
Dropped the throw since it can take down the actor loop. The Error was an AssertionError from the test's mismatched addPort args; it now just logs a warning so it's not silent.
Yicong-Huang
left a comment
There was a problem hiding this comment.
The src code seems to have a bigger issue. we should not make source code less robust to satisfy test cases...
|
Reverted the re-throw. Fixed the real root cause: the test passed mismatched |
|
@Ma77Ball as discussed, I think this is test-only issue. please revert the change on the source code. thanks |
|
@Yicong-Huang I pushed the changes please continue the review when available |
What changes were proposed in this PR?
WorkerSpec's input-portAssignPortRequestnow passes an emptystorageUris(List()) matching its emptypartitionings, satisfyingInputManager.addPort'ssizeinvariant, so the input port registers and the three worker tests pass deterministically.Any related issues, documentation, discussions?
Closes: #6036
How was this PR tested?
sbt "WorkflowExecutionService/testOnly *WorkerSpec"; expectTests: succeeded 3, failed 0.assertion failed. Before this fix the count is 3 (the assertion fired and was swallowed every run); after it is 0. Verified locally under Java 17.Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8 in compliance with ASF