test(amber): cover the runtime's serde bootstrap and worker startup - #7663
Merged
Conversation
AmberRuntime had no spec. It owns the actor system the rest of amber leans on, the serialization extension every message goes through, and the scheduling helpers services use for recurring work - and none of it was verified. Adds 15 tests, taking the file from 12.20% to 65.85% of lines. Nothing needs a cluster: startActorWorker(None) builds its whole system with artery on port 0 in under 100ms, makes no network call, and tears down again, so the worker startup path is drivable end to end. Covered: the serde bootstrap and its memoization, extending an installed system rather than replacing it, both scheduling helpers including cancellation and the recurring delay, the seed-node URI and master address, the artery host binding, the config fallback chain, and the two child actors with their dead-letter subscription. The spec saves and restores AmberRuntime's globals by reflection, the same way eight sibling specs do, and terminates every system it starts; run alongside ClusterListenerSpec and SessionStateSpec, all 21 pass. Left uncovered: getNodeIpAddress and startActorMaster, which bind real network interfaces. No production file is touched.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 349 | 0.213 | 27,776/33,948/33,948 us | 🟢 -12.0% / 🔴 +118.1% |
| 🔴 | bs=100 sw=10 sl=64 | 749 | 0.457 | 131,353/162,507/162,507 us | 🔴 +8.6% / 🔴 +51.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 867 | 0.529 | 1,147,592/1,270,796/1,270,796 us | ⚪ within ±5% / 🔴 +22.6% |
Baseline details
Latest main 8e80187 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 349 tuples/sec | 375 tuples/sec | 774.89 tuples/sec | -6.9% | -55.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.213 MB/s | 0.229 MB/s | 0.473 MB/s | -7.0% | -55.0% |
| bs=10 sw=10 sl=64 | p50 | 27,776 us | 26,148 us | 12,738 us | +6.2% | +118.1% |
| bs=10 sw=10 sl=64 | p95 | 33,948 us | 38,574 us | 16,348 us | -12.0% | +107.7% |
| bs=10 sw=10 sl=64 | p99 | 33,948 us | 38,574 us | 18,848 us | -12.0% | +80.1% |
| bs=100 sw=10 sl=64 | throughput | 749 tuples/sec | 791 tuples/sec | 1,005 tuples/sec | -5.3% | -25.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.457 MB/s | 0.483 MB/s | 0.613 MB/s | -5.4% | -25.5% |
| bs=100 sw=10 sl=64 | p50 | 131,353 us | 120,958 us | 100,970 us | +8.6% | +30.1% |
| bs=100 sw=10 sl=64 | p95 | 162,507 us | 166,345 us | 107,605 us | -2.3% | +51.0% |
| bs=100 sw=10 sl=64 | p99 | 162,507 us | 166,345 us | 116,429 us | -2.3% | +39.6% |
| bs=1000 sw=10 sl=64 | throughput | 867 tuples/sec | 883 tuples/sec | 1,030 tuples/sec | -1.8% | -15.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.529 MB/s | 0.539 MB/s | 0.629 MB/s | -1.9% | -15.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,147,592 us | 1,122,125 us | 991,433 us | +2.3% | +15.8% |
| bs=1000 sw=10 sl=64 | p95 | 1,270,796 us | 1,264,503 us | 1,036,668 us | +0.5% | +22.6% |
| bs=1000 sw=10 sl=64 | p99 | 1,270,796 us | 1,264,503 us | 1,070,470 us | +0.5% | +18.7% |
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,573.34,200,128000,349,0.213,27776.44,33948.01,33948.01
1,100,10,64,20,2669.00,2000,1280000,749,0.457,131352.95,162507.42,162507.42
2,1000,10,64,20,23080.18,20000,12800000,867,0.529,1147592.08,1270795.80,1270795.80
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7663 +/- ##
============================================
+ Coverage 90.13% 90.15% +0.02%
+ Complexity 4414 4410 -4
============================================
Files 1174 1174
Lines 46964 46964
Branches 5262 5262
============================================
+ Hits 42329 42342 +13
+ Misses 2877 2860 -17
- Partials 1758 1762 +4
*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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
amber/src/test/scala/org/apache/texera/amber/engine/common/AmberRuntimeSpec.scala:323
workerStartuprestores_actorSystem/_serdebut leaves other globals (e.g.,AmberConfig.masterNodeAddr,ClusterListener.numWorkerNodesInCluster) potentially mutated untilafterAll. Restoring these inside the fixture’sfinallykeeps the cross-suite visibility window as short as possible (seeClusterListenerSpec’s pattern).
} finally {
setAmberRuntimeField("_actorSystem", testSystem)
setAmberRuntimeField("_serde", testSerde)
if (created != null) TestKit.shutdownActorSystem(created)
}
amber/src/test/scala/org/apache/texera/amber/engine/common/AmberRuntimeSpec.scala:292
workerStartupmutates JVM-globalAmberConfig.masterNodeAddrbut doesn’t restore it in the fixture’sfinally. Since amber suites can run concurrently in one JVM, other suites can observe the mutated address for the remainder of this suite (untilafterAll), which is longer than one test.
This issue also appears on line 319 of the same file.
private lazy val workerStartup: WorkerStartup = {
// A sentinel is mandatory: AmberConfig.masterNodeAddr's DEFAULT is already
// Address("pekko", "Amber", "localhost", 2552), so without this the
// masterNodeAddr case would assert a value that held before the call.
AmberConfig.masterNodeAddr = Address("pekko", "Sentinel", "nowhere", 1)
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?
AmberRuntimehad no spec. It owns the actor system the rest of amber leans on, the serialization extension every message goes through, and the scheduling helpers services use for recurring work.Adds 15 tests, taking the file from 12.20% to 65.85% of lines (5/41 → 27/41, branches 1/8 → 5/8).
Nothing needs a cluster:
startActorWorker(None)builds its whole system with artery on port 0 in under 100 ms, makes no network call, and tears down again — so the worker startup path is drivable end to end.Covered: the serde bootstrap and its memoization, extending an installed system rather than replacing it, both scheduling helpers including cancellation and the recurring delay, the seed-node URI and master address, the artery host binding, the config fallback chain, and the two child actors with their dead-letter subscription.
Cross-suite safety
This matters more than the coverage number. amber sets neither
Test / forknorTest / parallelExecution := false, so every suite shares one JVM — and this file is the global state others depend on. The spec saves and restores_actorSystem,_serdeandAmberConfig.masterNodeAddrby reflection, the way eight sibling specs already do, and terminates every system it starts.Verified rather than asserted: run alongside
ClusterListenerSpecandSessionStateSpec, all 21 tests pass. Review also drove twelve runtime-touching suites in one shared JVM with a sentinel system and address installed beforehand, and confirmed both were handed back untouched.Verification
21 mutations applied one at a time and reverted, production diff empty each time.
Review then found three survivors, and two of the three are the same failure mode:
pekkoConfigartery.transport == "tcp", which is pekko's ownreference.confdefault — it holds withpekkoConfigabsent entirelycluster.confsuppliessystem.name == "Amber", not identityeqClusterListenerswapped forDeadLetterMonitorActorat the same pathActorRefexposes no class, and nothing exchanges a message only one answersThe first of those also exposed a measurement error worth stating: the build reported that mutation as killed, but the kill came from an earlier assertion in the same test failing first (
actor.provider == "cluster", which is genuine), so the transport line was never evaluated. A red test is not evidence that the assertion you care about pins anything.One further mutation survived during the build and was resolved by deleting the assertion rather than keeping it:
cluster.confalready setscanonical.port = 0, so the production override of it is unobservable.Deliberately not included
getNodeIpAddressandstartActorMasterbind real network interfaces. One partial branch remains at themainNodeAddress.isDefinedguard, whose true arm needs that same address lookup.No production file is touched.
Any related issues, documentation, discussions?
Closes #7662
How was this PR tested?
Test/scalafmtCheckandTest/scalafix --checkboth pass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)