ci: add a computing-unit-master boot smoke test in amber-integration#6377
Open
mengw15 wants to merge 1 commit into
Open
ci: add a computing-unit-master boot smoke test in amber-integration#6377mengw15 wants to merge 1 commit into
mengw15 wants to merge 1 commit into
Conversation
amber-integration builds and unzips the amber dist to boot-smoke-test texera-web (apache#6319), but computing-unit-master -- the other server from the same WorkflowExecutionService/dist (bin/computing-unit-master, port 8085) -- is never started anywhere in CI, so a runtime classpath/linkage regression (the apache#6204 class of bug) in it would pass build and unit tests and only surface at deploy. Add one more boot step, reusing the dist already built and unzipped for the texera-web boot. Like texera-web it boots on postgres only: main starts a local Pekko actor master (clusterMode defaults to false, so no external seed and the cluster-only checkip.amazonaws.com lookup is skipped) and run() calls SqlServer.initConnection; the boot-time result-store cleanup is gated on ApplicationConfig.cleanupAllExecutionResults, which defaults to false, so no iceberg/S3 access. ubuntu-only, mirroring the texera-web step. Closes apache#6344.
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 #6377 +/- ##
============================================
- Coverage 66.04% 66.04% -0.01%
+ Complexity 3380 3378 -2
============================================
Files 1142 1142
Lines 44980 44980
Branches 4948 4948
============================================
- Hits 29709 29705 -4
- Misses 13631 13632 +1
- Partials 1640 1643 +3
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the existing amber-integration CI workflow to also boot-smoke-test the packaged computing-unit-master launcher from the already-built Amber dist, catching runtime classpath/linkage regressions that won’t surface in compile/unit tests.
Changes:
- Add a new
amber-integrationstep (Ubuntu-only) to run.github/scripts/smoke-boot.shagainst/tmp/dists/amber-*/bin/computing-unit-masterand wait for port8085to reach LISTEN. - Reuse the existing dist build/unzip already performed for the
texera-webboot smoke test (no additional build work).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 483 | 0.295 | 19,290/33,947/33,947 us | 🔴 +14.8% / 🔴 +133.0% |
| 🟢 | bs=100 sw=10 sl=64 | 1,202 | 0.734 | 83,391/96,146/96,146 us | 🟢 -19.9% / 🟢 -13.5% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,443 | 0.88 | 690,012/732,699/732,699 us | ⚪ within ±5% / 🟢 +31.9% |
Baseline details
Latest main afa8af2 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 483 tuples/sec | 528 tuples/sec | 825.74 tuples/sec | -8.5% | -41.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.295 MB/s | 0.322 MB/s | 0.504 MB/s | -8.4% | -41.5% |
| bs=10 sw=10 sl=64 | p50 | 19,290 us | 16,932 us | 12,003 us | +13.9% | +60.7% |
| bs=10 sw=10 sl=64 | p95 | 33,947 us | 29,583 us | 14,566 us | +14.8% | +133.0% |
| bs=10 sw=10 sl=64 | p99 | 33,947 us | 29,583 us | 18,190 us | +14.8% | +86.6% |
| bs=100 sw=10 sl=64 | throughput | 1,202 tuples/sec | 1,192 tuples/sec | 1,072 tuples/sec | +0.8% | +12.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.734 MB/s | 0.727 MB/s | 0.654 MB/s | +1.0% | +12.2% |
| bs=100 sw=10 sl=64 | p50 | 83,391 us | 79,921 us | 94,522 us | +4.3% | -11.8% |
| bs=100 sw=10 sl=64 | p95 | 96,146 us | 120,106 us | 101,111 us | -19.9% | -4.9% |
| bs=100 sw=10 sl=64 | p99 | 96,146 us | 120,106 us | 111,122 us | -19.9% | -13.5% |
| bs=1000 sw=10 sl=64 | throughput | 1,443 tuples/sec | 1,405 tuples/sec | 1,094 tuples/sec | +2.7% | +31.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.88 MB/s | 0.858 MB/s | 0.668 MB/s | +2.6% | +31.8% |
| bs=1000 sw=10 sl=64 | p50 | 690,012 us | 709,978 us | 934,546 us | -2.8% | -26.2% |
| bs=1000 sw=10 sl=64 | p95 | 732,699 us | 760,502 us | 975,104 us | -3.7% | -24.9% |
| bs=1000 sw=10 sl=64 | p99 | 732,699 us | 760,502 us | 1,005,414 us | -3.7% | -27.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,414.30,200,128000,483,0.295,19290.04,33946.65,33946.65
1,100,10,64,20,1663.72,2000,1280000,1202,0.734,83390.65,96146.39,96146.39
2,1000,10,64,20,13864.47,20000,12800000,1443,0.880,690012.16,732698.88,732698.88
Comment on lines
+642
to
+654
| - name: Smoke-test computing-unit-master boots | ||
| # computing-unit-master boots on postgres only, like texera-web: main | ||
| # starts a LOCAL Pekko actor master (clusterMode defaults to false with | ||
| # no --cluster arg, so no external seed/discovery and the cluster-only | ||
| # checkip.amazonaws.com lookup is never reached), then run() calls | ||
| # SqlServer.initConnection against the postgres provisioned above. The | ||
| # boot-time result-store cleanup is gated on | ||
| # ApplicationConfig.cleanupAllExecutionResults, which defaults to false, | ||
| # so no iceberg / S3 access. Config resolves via Utils.amberHomePath to | ||
| # amber/src/main/resources/computing-unit-master-config.yml (port 8085). | ||
| # Reuses the amber dist built + unzipped above for the texera-web boot. | ||
| if: matrix.os == 'ubuntu-latest' | ||
| run: .github/scripts/smoke-boot.sh "/tmp/dists/amber-*/bin/computing-unit-master" 8085 |
Contributor
There was a problem hiding this comment.
do we also need to smoke test the CU worker node ?
Contributor
Author
There was a problem hiding this comment.
Looked at ComputingUnitWorker — it doesn't fit this smoke test the same way:
- No fixed port to probe.
startActorWorkerbindsartery.canonical.port = 0(random) and runs no HTTP server;smoke-boot.shneeds a known port (the master works because of its Dropwizard:8085). - A worker's health = "joined the cluster," which needs a master/seed at
:2552first — standalone it just retries and stays up, so "didn't crash" proves nothing. A real test is master + worker + a cluster-membership assertion. - Linkage coverage is mostly shared — same amber dist/classpath; the worker's only unique path is
startActorWorkervsstartActorMaster.
So I'd scope this PR to the master.
Contributor
There was a problem hiding this comment.
we can do it later. can we open a ticket to track this todo?
6 tasks
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?
amber-integrationalready builds and unzips the amber dist to boot-smoke-testtexera-web(#6319), butcomputing-unit-master— the other server produced by the sameWorkflowExecutionService/dist(bin/computing-unit-master, port 8085) — is not started anywhere in CI. It compiles and unit-tests clean, so a runtime classpath/linkage regression (the #6204 class of bug:NoClassDefFoundErrorfrom a dependency conflict that only manifests when the packaged launcher actually runs) would pass build and merge, surfacing only at deploy.amber-integration, right after thetexera-webboot, reusing the amber dist already built and unzipped there — no extra build. It launchesbin/computing-unit-mastervia.github/scripts/smoke-boot.shand asserts the process reaches a listening state on :8085 without crashing.It boots on postgres only, which
amber-integrationalready provisions:mainstarts a local Pekko actor master —clusterModedefaults tofalse(no--clusterarg), soAmberRuntime.startActorMaster(false)self-seeds a single node with no external discovery, and the cluster-onlygetNodeIpAddress(checkip.amazonaws.com) call is never reached — thenrun()callsSqlServer.initConnection.ApplicationConfig.cleanupAllExecutionResults, which defaults tofalse(asserted inApplicationConfigSpec), so no iceberg/S3 access on boot.Utils.amberHomePathtoamber/src/main/resources/computing-unit-master-config.yml(port 8085).ubuntu-latestonly, mirroring thetexera-webstep (the boot is pure-JVM and OS-independent).Any related issues, documentation, discussions?
Closes #6344. Mirrors the
texera-webboot test from #6319 and the platform-service boot tests from #6274; smoke script from #6220. Suggested by @Yicong-Huang in the #6319 review.How was this PR tested?
amber-integration(ubuntu-latest) runs the new step in this PR;bin/computing-unit-masterboots and reaches LISTEN on :8085.ComputingUnitMaster.main/run:clusterModedefaultfalse→ local actor master,cleanupAllExecutionResultsdefaultfalse→ no result-store cleanup,SqlServer.initConnection→ postgres only.texera-webboot (same dist, samesmoke-boot.sh, sameubuntu-onlyguard).Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-8)