refactor(computing-unit-managing-service): share CU listing helpers and batch k8s calls#6853
Open
kunwp1 wants to merge 4 commits into
Open
refactor(computing-unit-managing-service): share CU listing helpers and batch k8s calls#6853kunwp1 wants to merge 4 commits into
kunwp1 wants to merge 4 commits into
Conversation
…nd batch k8s calls Centralize the computing-unit listing logic in ComputingUnitHelpers so the per-user listing endpoint routes through a single, reusable implementation (also reused by a follow-up admin endpoint): - Add resolveOwnerInfo, partitionLiveUnits, reconcileVanishedKubernetesUnits, buildDashboardUnit, and the isKubernetes / podPhasesFor / podMetricsFor guards. - Remove the duplicated private getComputingUnitStatus / getComputingUnitMetrics from ComputingUnitManagingResource; callers use the shared helpers. - Add bulk KubernetesClient.getAllPodPhases / getAllPodMetrics (one namespace-wide list()/top() each) plus a shared containerUsage helper and a fetchPodMetricsItems extraction; listComputingUnits now resolves pod status/metrics in bulk instead of probing podExists twice per unit and re-fetching top() per unit. - Add a package-private KubernetesClient test seam (setClientForTesting) so the fabric8 client can be stubbed in tests; not used in production. API response behavior is unchanged; per-user reconciliation now runs over cuid-deduplicated units, reducing redundant DB updates. Tests: ComputingUnitHelpersSpec (embedded Postgres via MockTexeraDB), KubernetesClientSpec (stubbed fabric8 client), and a new ComputingUnitManagingResourceSpec driving the per-user endpoints over the embedded DB. build.sbt gains DAO/Auth test artifacts and serial test execution. Generated-by: Claude Code, Claude Opus 4.8
github-actions
Bot
added
dependencies
Contributor
Automated Reviewer SuggestionsBased on the
|
This was referenced Jul 24, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6853 +/- ##
============================================
+ Coverage 69.77% 71.55% +1.78%
- Complexity 3379 3665 +286
============================================
Files 1142 1145 +3
Lines 44828 44978 +150
Branches 4948 4963 +15
============================================
+ Hits 31277 32184 +907
+ Misses 11895 11029 -866
- Partials 1656 1765 +109
*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
Author
|
/request-review @mengw15 |
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🟢 | bs=10 sw=10 sl=64 | 392 | 0.239 | 25,161/30,597/30,597 us | 🟢 -11.6% / 🔴 +96.3% |
| 🔴 | bs=100 sw=10 sl=64 | 775 | 0.473 | 125,462/156,482/156,482 us | 🔴 +8.4% / 🔴 +43.4% |
| ⚪ | bs=1000 sw=10 sl=64 | 910 | 0.555 | 1,102,190/1,146,225/1,146,225 us | ⚪ within ±5% / 🔴 +10.0% |
Baseline details
Latest main 17cdba4 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 392 tuples/sec | 399 tuples/sec | 754.55 tuples/sec | -1.8% | -48.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.239 MB/s | 0.243 MB/s | 0.461 MB/s | -1.6% | -48.1% |
| bs=10 sw=10 sl=64 | p50 | 25,161 us | 24,812 us | 12,816 us | +1.4% | +96.3% |
| bs=10 sw=10 sl=64 | p95 | 30,597 us | 34,617 us | 16,594 us | -11.6% | +84.4% |
| bs=10 sw=10 sl=64 | p99 | 30,597 us | 34,617 us | 19,806 us | -11.6% | +54.5% |
| bs=100 sw=10 sl=64 | throughput | 775 tuples/sec | 814 tuples/sec | 969.38 tuples/sec | -4.8% | -20.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.473 MB/s | 0.497 MB/s | 0.592 MB/s | -4.8% | -20.1% |
| bs=100 sw=10 sl=64 | p50 | 125,462 us | 122,209 us | 103,584 us | +2.7% | +21.1% |
| bs=100 sw=10 sl=64 | p95 | 156,482 us | 144,307 us | 109,097 us | +8.4% | +43.4% |
| bs=100 sw=10 sl=64 | p99 | 156,482 us | 144,307 us | 117,304 us | +8.4% | +33.4% |
| bs=1000 sw=10 sl=64 | throughput | 910 tuples/sec | 917 tuples/sec | 1,004 tuples/sec | -0.8% | -9.3% |
| bs=1000 sw=10 sl=64 | MB/s | 0.555 MB/s | 0.56 MB/s | 0.613 MB/s | -0.9% | -9.4% |
| bs=1000 sw=10 sl=64 | p50 | 1,102,190 us | 1,092,712 us | 1,002,357 us | +0.9% | +10.0% |
| bs=1000 sw=10 sl=64 | p95 | 1,146,225 us | 1,119,218 us | 1,046,463 us | +2.4% | +9.5% |
| bs=1000 sw=10 sl=64 | p99 | 1,146,225 us | 1,119,218 us | 1,073,661 us | +2.4% | +6.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,510.24,200,128000,392,0.239,25161.08,30597.11,30597.11
1,100,10,64,20,2580.25,2000,1280000,775,0.473,125462.34,156481.54,156481.54
2,1000,10,64,20,21988.47,20000,12800000,910,0.555,1102190.44,1146224.67,1146224.67
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the computing-unit managing service’s per-user CU listing to reuse shared helper logic and reduce Kubernetes API round-trips by fetching pod phase/metrics in bulk, while keeping the API response shape unchanged. This lays groundwork for the upcoming admin computing-unit listing endpoint.
Changes:
- Centralized CU dashboard-row construction and owner/status/metrics resolution in
ComputingUnitHelpers, including bulk-friendly variants and vanished-pod reconciliation. - Added bulk Kubernetes queries in
KubernetesClient(getAllPodPhases,getAllPodMetrics) plus a test seam (setClientForTesting) to enable stubbing fabric8 without a cluster. - Added/expanded unit tests for the new helper and bulk K8s paths, and introduced a resource-level spec for per-user endpoints using embedded Postgres.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| computing-unit-managing-service/src/main/scala/org/apache/texera/service/util/KubernetesClient.scala | Adds bulk namespace-wide pod phase/metrics fetchers and a package-private test seam for the fabric8 client. |
| computing-unit-managing-service/src/main/scala/org/apache/texera/service/util/ComputingUnitHelpers.scala | Introduces shared CU listing helpers: owner info resolution, bulk status/metrics, live/vanished partitioning, reconciliation, and dashboard-row builder. |
| computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala | Refactors per-user listing to dedupe by cuid and use bulk pod phase/metrics maps plus shared helper methods. |
| computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala | Adds Mockito-based fabric8 stubs and tests covering bulk K8s wrappers and helper guard behavior. |
| computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/ComputingUnitHelpersSpec.scala | Expands helper coverage for bulk variants, reconciliation behavior, and DB-backed owner-info resolution. |
| computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitManagingResourceSpec.scala | Adds embedded-DB resource-level tests for per-user endpoints using local units (no K8s calls). |
| build.sbt | Wires test-scope dependencies for embedded DB reuse and forces serial test execution for this module due to JVM-wide singletons. |
Comments suppressed due to low confidence (2)
computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala:124
- afterAll() should call super.afterAll(), ideally in a finally block, to ensure other traits' teardown runs even if resetting the KubernetesClient fails.
override protected def afterAll(): Unit = {
// Restore a real client so subsequent suites don't reuse the mock.
KubernetesClient.setClientForTesting(new KubernetesClientBuilder().build())
}
computing-unit-managing-service/src/test/scala/org/apache/texera/service/resource/ComputingUnitManagingResourceSpec.scala:73
- afterAll() should call super.afterAll(), preferably in a finally block, to ensure proper teardown if additional traits are mixed in later.
override protected def afterAll(): Unit = shutdownDB()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Kunwoo (Chris) <143021053+kunwp1@users.noreply.github.com>
…cle hooks Complete and correct the Copilot-suggested fix on apache#6853. The auto-pushed commit only touched KubernetesClientSpec.beforeAll (dedenting the override so it fails scalafmtCheck) and left the other sites unaddressed. - Restore correct indentation in KubernetesClientSpec.beforeAll. - Call super.afterAll() (via try/finally) in KubernetesClientSpec.afterAll. - Call super.beforeAll()/super.afterAll() in ComputingUnitHelpersSpec and ComputingUnitManagingResourceSpec. So lifecycle code from other mixed-in traits is not skipped if the trait stack changes; matches the majority convention across the repo's suites. No behavior change (MockTexeraDB defines no lifecycle hooks). scalafmtCheck clean; the three specs pass. Generated-by: Claude Code, Claude Opus 4.8
kunwp1
added a commit
to kunwp1/texera
that referenced
this pull request
Jul 24, 2026
…cle hooks Mirror the apache#6853 review fix onto the stacked admin PR so the whole contribution is uniform: beforeAll/afterAll overrides call super.beforeAll()/super.afterAll() (teardown via try/finally) in the shared specs and in AdminComputingUnitResourceSpec. No behavior change. Generated-by: Claude Code, Claude Opus 4.8
… var; guard null pod status Address review feedback on apache#6853: - Remove the mutable `var client` / `setClientForTesting` test seam. The production singleton is a `val` again; the fabric8 -> map transforms are extracted as pure package-private helpers (phasesByPodName, metricsByPodName) and tested with builder-constructed model objects, so KubernetesClientSpec no longer stubs the client or mutates a JVM-wide global. Drop the low-value fabric8 passthrough tests; move the cluster-free podPhasesFor/podMetricsFor guard test to ComputingUnitHelpersSpec. - Guard the single-unit getComputingUnitStatus against a pod that has no status yet (Option(getStatus)), matching the bulk getAllPodPhases path, so getComputingUnitInfo no longer risks an NPE. - Update the build.sbt serial-execution note: only MockTexeraDB's SqlServer singleton now requires it. Generated-by: Claude Code, Claude Opus 4.8
aicam
approved these changes
Jul 25, 2026
| ): ComputingUnitState = { | ||
| unit.getType match { | ||
| case WorkflowComputingUnitTypeEnum.local => | ||
| Running |
Contributor
There was a problem hiding this comment.
I think we should check if computing unit microservice is running. For example if I create a local CU, then turn off computing unit master service, it still shows its running but its actually off. Not a blocker since local deployment is not our standard, just a note.
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?
Refactor the per-user computing-unit listing so its status/metrics logic lives in one reusable place and hits Kubernetes in bulk. No API response changes; this is groundwork for the admin listing endpoint (stacked follow-up PR).
ComputingUnitHelpers(resolveOwnerInfo,partitionLiveUnits,reconcileVanishedKubernetesUnits,buildDashboardUnit, plus theisKubernetes/podPhasesFor/podMetricsForguards), and remove the duplicated private status/metrics helpers fromComputingUnitManagingResource.KubernetesClient.getAllPodPhases/getAllPodMetrics(one namespace-widelist()/top()each) and a sharedcontainerUsagehelper;listComputingUnitsnow resolves pod status/metrics in bulk instead of probing pod existence twice per unit and re-fetchingtop()per unit.KubernetesClienttest seam (setClientForTesting) so the fabric8 client can be stubbed in tests; unused in production.Any related issues, documentation, discussions?
Part of #6476 (Admin Computing Units Dashboard); prepares the shared listing helpers reused by the admin endpoint in the stacked follow-up. Originates from discussion #6322.
How was this PR tested?
Added
ComputingUnitManagingResourceSpecand extendedComputingUnitHelpersSpec/KubernetesClientSpec(embedded Postgres viaMockTexeraDB; stubbed fabric8 client for the bulk k8s paths).Ran
sbt 'ComputingUnitManagingService/testOnly org.apache.texera.service.util.ComputingUnitHelpersSpec org.apache.texera.service.util.KubernetesClientSpec org.apache.texera.service.resource.ComputingUnitManagingResourceSpec'(34 tests, 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