feat(amber): expose the warehouse owner in DashboardWarehouse - #7745
Merged
Conversation
GET /warehouse/status returned DashboardWarehouse without owner information, so the warehouse dashboard tab and picker could only render the owner avatar from the currently signed-in user -- correct only while warehouses are strictly per-user, and wrong as soon as they can be shared. Mirror how computing units model this: add ownerName / ownerAvatar to DashboardWarehouse, resolved per entry from the user table (null when the user has no name or avatar set), batched over the distinct owner uids of a listing. The UI can then bind each entry to its own owner instead of the session user. WarehouseResourceSpec asserts the fields on both mapping paths: create returns the caller's name with a null avatar for the avatar-less fixture user, and status resolves another user's name and avatar per entry. Part of apache#6870, follow-up to apache#6932. Closes apache#7743.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds per-warehouse owner metadata to dashboard API responses.
Changes:
- Batches owner name/avatar resolution from the user table.
- Populates owner fields for warehouse listing and creation responses.
- Tests populated and null owner metadata.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
WarehouseResource.scala |
Adds and resolves owner fields. |
WarehouseResourceSpec.scala |
Verifies owner metadata mappings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 391 | 0.238 | 25,435/30,548/30,548 us | 🔴 +20.0% / 🔴 +99.5% |
| 🔴 | bs=100 sw=10 sl=64 | 797 | 0.486 | 123,894/144,281/144,281 us | 🔴 +6.6% / 🔴 +35.1% |
| ⚪ | bs=1000 sw=10 sl=64 | 930 | 0.567 | 1,074,475/1,164,261/1,164,261 us | ⚪ within ±5% / 🔴 +14.9% |
Baseline details
Latest main 60300e3 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 391 tuples/sec | 419 tuples/sec | 771.02 tuples/sec | -6.7% | -49.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.238 MB/s | 0.256 MB/s | 0.471 MB/s | -7.0% | -49.4% |
| bs=10 sw=10 sl=64 | p50 | 25,435 us | 21,200 us | 12,749 us | +20.0% | +99.5% |
| bs=10 sw=10 sl=64 | p95 | 30,548 us | 33,998 us | 15,594 us | -10.1% | +95.9% |
| bs=10 sw=10 sl=64 | p99 | 30,548 us | 33,998 us | 19,320 us | -10.1% | +58.1% |
| bs=100 sw=10 sl=64 | throughput | 797 tuples/sec | 834 tuples/sec | 997.05 tuples/sec | -4.4% | -20.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.486 MB/s | 0.509 MB/s | 0.609 MB/s | -4.5% | -20.1% |
| bs=100 sw=10 sl=64 | p50 | 123,894 us | 116,243 us | 100,339 us | +6.6% | +23.5% |
| bs=100 sw=10 sl=64 | p95 | 144,281 us | 151,279 us | 106,781 us | -4.6% | +35.1% |
| bs=100 sw=10 sl=64 | p99 | 144,281 us | 151,279 us | 113,101 us | -4.6% | +27.6% |
| bs=1000 sw=10 sl=64 | throughput | 930 tuples/sec | 928 tuples/sec | 1,037 tuples/sec | +0.2% | -10.3% |
| bs=1000 sw=10 sl=64 | MB/s | 0.567 MB/s | 0.566 MB/s | 0.633 MB/s | +0.2% | -10.4% |
| bs=1000 sw=10 sl=64 | p50 | 1,074,475 us | 1,069,727 us | 971,388 us | +0.4% | +10.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,164,261 us | 1,177,814 us | 1,013,249 us | -1.2% | +14.9% |
| bs=1000 sw=10 sl=64 | p99 | 1,164,261 us | 1,177,814 us | 1,039,879 us | -1.2% | +12.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,512.14,200,128000,391,0.238,25435.19,30548.27,30548.27
1,100,10,64,20,2509.84,2000,1280000,797,0.486,123893.77,144281.22,144281.22
2,1000,10,64,20,21511.74,20000,12800000,930,0.567,1074475.36,1164260.62,1164260.62
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7745 +/- ##
============================================
+ Coverage 91.02% 91.22% +0.19%
- Complexity 4454 4468 +14
============================================
Files 1174 1171 -3
Lines 47146 47124 -22
Branches 5287 5286 -1
============================================
+ Hits 42916 42988 +72
+ Misses 2550 2458 -92
+ Partials 1680 1678 -2
*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:
|
kunwp1
reviewed
Aug 19, 2026
Review feedback on the owner fields: The listing fetched warehouses and then looked their owners up in a second query. Join instead, so one round trip carries both. `create` went to the database at all, though the caller is the owner it is resolving and SessionUser already holds their display info -- it now reads from there. What remains of the resolution is turning an unset name or avatar into null, so that is all `ownerOf` does. The `(null, null)` fallback a reviewer asked to name is gone rather than named: with the left join supplying nulls directly and `create` reading the session, nothing is left that has to invent an absent owner.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala:205
- The create response takes owner data from the JWT-backed
SessionUser, whilestatus()reads the user table. JWT claims can be stale (for example, an admin can update the user's name while their token remains valid), so the POST response can return a different owner than an immediate GET and does not satisfy the per-entry database resolution. Read the owner row here as well; a regression test can mutate the persisted user after constructing the session.
// The caller owns what they just created, and SessionUser already carries their
// display info -- no lookup needed.
toDashboardWarehouse(row, ownerOf(current_user.getUser))
kunwp1
approved these changes
Aug 19, 2026
kunwp1
left a comment
kunwp1
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Can you also update the PR description because "batched over the distinct owner uids of a listing (one query per request)" this is no longer valid.
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?
GET /warehouse/statusreturnsDashboardWarehouse(whid, name, warehouseName, flavor, createdAtMillis)with no owner information, so the warehouse dashboard tab and picker (#6933) can only render the owner avatar from the currently signed-in user. That is correct only while warehouses are strictly per-user, and would show the wrong person as soon as warehouses can be shared. Computing units already model this properly:DashboardWorkflowComputingUnitcarriesownerName/ownerAvatarresolved per entry.ownerNameandownerAvatartoDashboardWarehouse, mirroring the computing-unit semantics: resolved from the user table per entry, null when the user has no name or avatar set.status()listing and thecreate()response.Any related issues, documentation, discussions?
Closes #7743. Part of #6870, follow-up to #6932; mirrors
DashboardWorkflowComputingUnit's owner fields.How was this PR tested?
WarehouseResourceSpec(embedded Postgres + stubbedLakekeeperClient, no external infra) asserts the fields on both mapping paths:createreturns the caller'sownerNamewith a nullownerAvatarfor the avatar-less fixture user, andstatusresolves another user's name and avatar per entry.WorkflowExecutionService/scalafmtCheck(main + Test) passes.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-8)