test(amber): replace commented-out PythonWorkflowWorkerSpec with pythonworker proxy unit tests - #7488
Merged
aglinxinyuan merged 1 commit intoAug 10, 2026
Conversation
…onworker proxy unit tests PythonWorkflowWorkerSpec.scala has been fully commented out for years and no longer compiles against today's APIs; the actor-level path it targeted is covered by the e2e tests. Replace it with real unit tests for the JVM side of the JVM<->Python Arrow Flight bridge, which had none: - PythonProxyServerSpec: handshake port promise + ok reply; control actions route ControlInvocation/ReturnInvocation to the output gateway on the control channel and ack with a little-endian credit; Data/State/ ECM puts are reassembled into DataFrame/StateFrame (loop envelope preserved)/EmbeddedControlMessage and acked with credits. - PythonProxyClientSpec: heartbeat-before-drain connection order; queued control/actor commands and Data/State/ECM payloads arrive as the right Flight actions/puts with intact payloads; queue-size acks drive getQueuedCredit; retry exhaustion and non-ack heartbeats abort with WorkflowRuntimeException; close() before connecting does not throw. Both specs stand in for the Python worker with plain Arrow Flight components, so no Python process is involved.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 365 | 0.223 | 25,921/36,759/36,759 us | 🔴 +16.0% / 🔴 +124.3% |
| 🟢 | bs=100 sw=10 sl=64 | 809 | 0.494 | 123,346/136,770/136,770 us | 🟢 -8.7% / 🔴 +21.6% |
| 🔴 | bs=1000 sw=10 sl=64 | 893 | 0.545 | 1,110,506/1,207,557/1,207,557 us | 🔴 +7.8% / 🔴 +12.2% |
Baseline details
Latest main 6790bf1 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 365 tuples/sec | 414 tuples/sec | 743.73 tuples/sec | -11.8% | -50.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.223 MB/s | 0.253 MB/s | 0.454 MB/s | -11.9% | -50.9% |
| bs=10 sw=10 sl=64 | p50 | 25,921 us | 23,904 us | 13,130 us | +8.4% | +97.4% |
| bs=10 sw=10 sl=64 | p95 | 36,759 us | 31,700 us | 16,391 us | +16.0% | +124.3% |
| bs=10 sw=10 sl=64 | p99 | 36,759 us | 31,700 us | 19,408 us | +16.0% | +89.4% |
| bs=100 sw=10 sl=64 | throughput | 809 tuples/sec | 785 tuples/sec | 944.04 tuples/sec | +3.1% | -14.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.494 MB/s | 0.479 MB/s | 0.576 MB/s | +3.1% | -14.3% |
| bs=100 sw=10 sl=64 | p50 | 123,346 us | 125,996 us | 105,853 us | -2.1% | +16.5% |
| bs=100 sw=10 sl=64 | p95 | 136,770 us | 149,744 us | 112,493 us | -8.7% | +21.6% |
| bs=100 sw=10 sl=64 | p99 | 136,770 us | 149,744 us | 122,200 us | -8.7% | +11.9% |
| bs=1000 sw=10 sl=64 | throughput | 893 tuples/sec | 919 tuples/sec | 972.22 tuples/sec | -2.8% | -8.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.545 MB/s | 0.561 MB/s | 0.593 MB/s | -2.9% | -8.2% |
| bs=1000 sw=10 sl=64 | p50 | 1,110,506 us | 1,083,958 us | 1,033,856 us | +2.4% | +7.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,207,557 us | 1,120,686 us | 1,076,083 us | +7.8% | +12.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,207,557 us | 1,120,686 us | 1,107,701 us | +7.8% | +9.0% |
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,548.14,200,128000,365,0.223,25920.71,36758.69,36758.69
1,100,10,64,20,2472.87,2000,1280000,809,0.494,123346.21,136770.14,136770.14
2,1000,10,64,20,22403.83,20000,12800000,893,0.545,1110506.27,1207556.77,1207556.77
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7488 +/- ##
============================================
+ Coverage 85.69% 85.96% +0.27%
- Complexity 4168 4210 +42
============================================
Files 1169 1169
Lines 46739 46739
Branches 5203 5203
============================================
+ Hits 40052 40181 +129
+ Misses 4974 4824 -150
- Partials 1713 1734 +21
*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?
#7447 proposed deleting
PythonWorkflowWorkerSpec.scala, which has been fully commented out for years and no longer compiles against today's APIs. Following the review feedback there (#7447 (comment)), this PR replaces the dead file with real unit tests instead of only deleting it.The commented-out spec drove
PythonWorkflowWorkerend-to-end, which needs a live Python process — that path is covered by the e2e tests. What can be unit-tested without Python is the JVM side of the JVM↔Python Arrow Flight bridge, which had no coverage until now:PythonProxyServerSpecPythonProxyServer/AmberProducerhandshakecompletes the port promise and repliesok;controlactions routeControlInvocation/ReturnInvocationto the output gateway on the control channel and ack with a little-endian credit value;Data/State/ECMputs are reassembled intoDataFrame/StateFrame(loop envelope preserved) /EmbeddedControlMessageand acked with creditsPythonProxyClientSpecPythonProxyClientControlInvocation/ReturnInvocation/ actor commands arrive ascontrol/actoractions with intact protobuf payloads;DataFrame/StateFrame/ ECM puts arrive under the rightPythonDataHeaderwith tuples, loop envelope, and bytes intact; queue-size acks updategetQueuedCredit; connection retries abort withWorkflowRuntimeException(no server listening, non-ackheartbeat);close()before any connection does not throwBoth specs stand in for the Python worker with plain Arrow Flight components (
FlightClient/NoOpFlightProducer), so no Python process is involved. Together with the existingWorkerBatchInternalQueueSpecandPythonWorkflowWorkerStartupConfigSpec, every class in thepythonworkerpackage except the actor itself now has unit coverage.The commented-out
PythonWorkflowWorkerSpec.scalais removed, superseded by these tests.Any related issues, documentation, discussions?
Supersedes #7447.
How was this PR tested?
This PR is tests-only. Both new specs pass locally:
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Fable 5)