feat(computing-unit-managing-service): add admin endpoint to list all computing units#6484
Draft
kunwp1 wants to merge 8 commits into
Draft
feat(computing-unit-managing-service): add admin endpoint to list all computing units#6484kunwp1 wants to merge 8 commits into
kunwp1 wants to merge 8 commits into
Conversation
… computing units Add an ADMIN-only GET /computing-unit/admin/list that returns every non-terminated computing unit across all users, backing the admin Computing Units dashboard. Row assembly is extracted into a pure buildDashboardUnits function so it is unit-testable without a database or Kubernetes. Register the resource and extend the service's startup access-control coverage check.
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 #6484 +/- ##
============================================
+ Coverage 69.77% 70.12% +0.35%
- Complexity 3379 3436 +57
============================================
Files 1142 1147 +5
Lines 44828 44956 +128
Branches 4948 4959 +11
============================================
+ Hits 31277 31525 +248
+ Misses 11895 11761 -134
- Partials 1656 1670 +14
*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:
|
…nd batch k8s calls Centralize computing-unit listing logic in ComputingUnitHelpers so the per-user and admin listing endpoints share one implementation: - Add resolveOwnerInfo, partitionLiveUnits, reconcileVanishedKubernetesUnits, and buildDashboardUnit; both listing endpoints now route through them. - Remove the byte-identical private getComputingUnitStatus/getComputingUnitMetrics from ComputingUnitManagingResource; single-unit callers use the shared helpers. - Add bulk KubernetesClient.getAllPodPhases / getAllPodMetrics (one namespace-wide list()/top() each) and a shared containerUsage helper; listComputingUnits now resolves pod status/metrics in bulk instead of per-unit (previously podExists was probed twice per unit and top() was re-fetched per unit). - Admin endpoint filters active units in SQL and reconciles vanished pods via the shared helper. Behavior of the API responses is unchanged; per-user reconciliation now runs over cuid-deduplicated units, reducing redundant DB updates without adding new ones.
Simplify the computing-unit listing logic shared by the per-user and admin endpoints: - Add a single ComputingUnitHelpers.isKubernetes predicate plus podPhasesFor/podMetricsFor helpers, collapsing the four copies of the "only hit the cluster when a k8s unit is present" guard. - partitionLiveUnits returns (live, vanished) via a negated predicate instead of partitioning into (vanished, live) and swapping the tuple. - Per-user listComputingUnits maps over the reconciled live units directly (privilege captured by cuid) instead of round-tripping through a Set[cuid], and resolves owner info from the live units to match the admin endpoint. - Extract KubernetesClient.fetchPodMetricsItems so getAllPodMetrics and getPodMetrics share one top() fetch chain. API response behavior is unchanged.
Bring patch coverage of the admin-listing change to 100% by testing the previously-uncovered database, Kubernetes, and bootstrap paths: - KubernetesClientSpec now injects a stubbed fabric8 client (via a small test seam) to exercise getAllPodPhases/getAllPodMetrics/getPodMetrics, getPodByName/podExists, and ComputingUnitHelpers.podPhasesFor/podMetricsFor. - ComputingUnitHelpersSpec covers resolveOwnerInfo and reconcileVanishedKubernetesUnits against an embedded Postgres (MockTexeraDB), plus the untyped/unknown branches of the bulk status/metrics helpers. - AdminComputingUnitResourceSpec drives listAllComputingUnits end-to-end over the embedded DB (terminated rows excluded, WRITE access, isOwner). - New ComputingUnitManagingResourceSpec drives getComputingUnitInfo, the metrics endpoint, and listComputingUnits over the embedded DB. - ComputingUnitManagingServiceRunSpec boots run() with a mocked Dropwizard environment and verifies the resources (incl. the admin resource) register. Supporting changes: - KubernetesClient: add a package-private setClientForTesting seam so the fabric8 client can be stubbed in tests (not used in production). - ComputingUnitManagingService: open the DB pool in initialize() rather than run() (matching the sibling services) so run() is testable without a DB. - ComputingUnitHelpers: express isKubernetes as a match and avoid tuple destructuring so branch coverage is complete. - build.sbt: depend on DAO/Auth test artifacts for MockTexeraDB and run the suites serially (they mutate JVM-wide singletons).
github-actions
Bot
added
dependencies
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 496 | 0.303 | 18,789/26,920/26,920 us | 🔴 +13.8% / 🔴 +72.9% |
| 🔴 | bs=100 sw=10 sl=64 | 1,024 | 0.625 | 95,862/128,268/128,268 us | 🔴 +13.9% / 🔴 +20.6% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,168 | 0.713 | 861,416/888,584/888,584 us | ⚪ within ±5% / 🟢 -15.8% |
Baseline details
Latest main e08b7f3 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 496 tuples/sec | 544 tuples/sec | 768.42 tuples/sec | -8.8% | -35.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.303 MB/s | 0.332 MB/s | 0.469 MB/s | -8.7% | -35.4% |
| bs=10 sw=10 sl=64 | p50 | 18,789 us | 18,533 us | 12,686 us | +1.4% | +48.1% |
| bs=10 sw=10 sl=64 | p95 | 26,920 us | 23,662 us | 15,574 us | +13.8% | +72.9% |
| bs=10 sw=10 sl=64 | p99 | 26,920 us | 23,662 us | 20,076 us | +13.8% | +34.1% |
| bs=100 sw=10 sl=64 | throughput | 1,024 tuples/sec | 1,089 tuples/sec | 1,001 tuples/sec | -6.0% | +2.3% |
| bs=100 sw=10 sl=64 | MB/s | 0.625 MB/s | 0.665 MB/s | 0.611 MB/s | -6.0% | +2.3% |
| bs=100 sw=10 sl=64 | p50 | 95,862 us | 87,280 us | 99,753 us | +9.8% | -3.9% |
| bs=100 sw=10 sl=64 | p95 | 128,268 us | 112,574 us | 106,344 us | +13.9% | +20.6% |
| bs=100 sw=10 sl=64 | p99 | 128,268 us | 112,574 us | 119,585 us | +13.9% | +7.3% |
| bs=1000 sw=10 sl=64 | throughput | 1,168 tuples/sec | 1,205 tuples/sec | 1,029 tuples/sec | -3.1% | +13.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.713 MB/s | 0.735 MB/s | 0.628 MB/s | -3.0% | +13.5% |
| bs=1000 sw=10 sl=64 | p50 | 861,416 us | 832,505 us | 979,178 us | +3.5% | -12.0% |
| bs=1000 sw=10 sl=64 | p95 | 888,584 us | 870,258 us | 1,021,692 us | +2.1% | -13.0% |
| bs=1000 sw=10 sl=64 | p99 | 888,584 us | 870,258 us | 1,055,014 us | +2.1% | -15.8% |
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,402.88,200,128000,496,0.303,18788.87,26919.73,26919.73
1,100,10,64,20,1952.47,2000,1280000,1024,0.625,95862.16,128267.84,128267.84
2,1000,10,64,20,17121.68,20000,12800000,1168,0.713,861415.74,888583.74,888583.74Drive ComputingUnitManagingService.run() in the run-spec so the resource registrations (including the admin resource) are covered. run() opens the real connection pool via SqlServer.initConnection, so the test connects only when Postgres is reachable at the configured JDBC URL (as provisioned in CI) and cancels otherwise, keeping local runs green without a database. Revert the earlier relocation of initConnection to initialize(): keeping it in run() leaves the service's diff minimal (no bootstrap lines become newly uncovered) while the run-spec above covers the registration path.
Small readability cleanups in the new specs (no behavior/coverage change): - KubernetesClientSpec: extract a pod(cuid, phase) helper, collapsing the duplicated PodBuilder blocks (list items + the present-pod stub). - ComputingUnitHelpersSpec: route localUnit/kubernetesUnit through a shared makeUnit(cuid, uid, type) (matching AdminComputingUnitResourceSpec), and hoist the repeated UserDao/WorkflowComputingUnitDao construction into lazy vals.
…ting code Trim the scaladocs and inline comments added by this PR to their essentials (keeping the non-obvious "why": O(1) round trips, shared-by-both-endpoints, null-collapse, pure/testable, survivors-only metrics). No code changes.
Rephrase the CU-listing comments to describe behavior instead of naming methods (getComputingUnitStatus/getAllPodPhases/getPodMetrics/getPodByName, etc.), so they don't drift when those methods are renamed. Wording only; the non-obvious rationale (O(1) round trips, pre-fetched maps, presence-by- pod-name, DB-gated run test) is preserved.
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
2 participants
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?
Adds an ADMIN-only endpoint that returns every non-terminated computing unit across all users. Needed for adding admin Computing Units dashboard.
The endpoint queries all units via
WorkflowComputingUnitDao, excludes rows with a non-nullterminate_time, joinsUserDaofor each owner's name and avatar, and reusesComputingUnitHelpers.getComputingUnitStatus/getComputingUnitMetricsfor status and metrics.The new resource is registered in
ComputingUnitManagingService.accessPrivilegeis reported asWRITEfor admin rows (an admin can manage any unit it can see), andisOwnerreflects whether the requesting admin happens to own the unit.Any related issues, documentation, discussions?
Closes #6477. Part of #6476 (Admin Computing Units Dashboard). Originates from discussion #6322.
How was this PR tested?
Added
AdminComputingUnitResourceSpecRan
sbt 'ComputingUnitManagingService/testOnly org.apache.texera.service.resource.AdminComputingUnitResourceSpec org.apache.texera.service.ComputingUnitManagingServiceRunSpec'(all green) andsbt 'ComputingUnitManagingService/scalafmtCheck; ComputingUnitManagingService/Test/scalafmtCheck'(clean).Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code, Claude Opus 4.8