Repository navigation
Conversation
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🟢 | bs=10 sw=10 sl=64 | 388 | 0.237 | 25,175/32,029/32,029 us | 🟢 -18.4% / 🔴 +107.6% |
| 🔴 | bs=100 sw=10 sl=64 | 786 | 0.48 | 126,162/146,877/146,877 us | 🔴 +8.8% / 🔴 +41.0% |
| ⚪ | bs=1000 sw=10 sl=64 | 912 | 0.557 | 1,094,496/1,183,740/1,183,740 us | ⚪ within ±5% / 🔴 +20.1% |
Baseline details
Latest main 7426431 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 388 tuples/sec | 392 tuples/sec | 822.11 tuples/sec | -1.0% | -52.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.237 MB/s | 0.239 MB/s | 0.502 MB/s | -0.8% | -52.8% |
| bs=10 sw=10 sl=64 | p50 | 25,175 us | 24,876 us | 12,129 us | +1.2% | +107.6% |
| bs=10 sw=10 sl=64 | p95 | 32,029 us | 39,232 us | 15,469 us | -18.4% | +107.1% |
| bs=10 sw=10 sl=64 | p99 | 32,029 us | 39,232 us | 18,357 us | -18.4% | +74.5% |
| bs=100 sw=10 sl=64 | throughput | 786 tuples/sec | 841 tuples/sec | 1,058 tuples/sec | -6.5% | -25.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.48 MB/s | 0.513 MB/s | 0.646 MB/s | -6.4% | -25.7% |
| bs=100 sw=10 sl=64 | p50 | 126,162 us | 117,485 us | 97,399 us | +7.4% | +29.5% |
| bs=100 sw=10 sl=64 | p95 | 146,877 us | 134,996 us | 104,184 us | +8.8% | +41.0% |
| bs=100 sw=10 sl=64 | p99 | 146,877 us | 134,996 us | 117,615 us | +8.8% | +24.9% |
| bs=1000 sw=10 sl=64 | throughput | 912 tuples/sec | 920 tuples/sec | 1,099 tuples/sec | -0.9% | -17.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.557 MB/s | 0.561 MB/s | 0.671 MB/s | -0.7% | -17.0% |
| bs=1000 sw=10 sl=64 | p50 | 1,094,496 us | 1,086,993 us | 946,743 us | +0.7% | +15.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,183,740 us | 1,137,898 us | 985,437 us | +4.0% | +20.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,183,740 us | 1,137,898 us | 1,012,622 us | +4.0% | +16.9% |
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,515.07,200,128000,388,0.237,25174.99,32029.38,32029.38
1,100,10,64,20,2544.79,2000,1280000,786,0.480,126161.85,146876.57,146876.57
2,1000,10,64,20,21931.00,20000,12800000,912,0.557,1094496.44,1183740.37,1183740.37
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8144 +/- ##
=========================================
Coverage 92.30% 92.31%
- Complexity 5046 5047 +1
=========================================
Files 1260 1260
Lines 53887 53887
Branches 6711 6711
=========================================
+ Hits 49743 49745 +2
Misses 2486 2486
+ Partials 1658 1656 -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:
|
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟢 0 must-fix · 0 advisory · 2 polish — a clean one-line fix; both notes are text-level and neither blocks the merge.
Polish: 2 quick touch-ups (see inline comments).
Verification trace
Checked that the exception swap actually changes the status rather than assuming it does. amber registers no ExceptionMapper of its own, so the old IllegalArgumentException hit Dropwizard's generic Throwable handler; javax.ws.rs.BadRequestException carries its own Response.status(BAD_REQUEST) and takes the CLIENT_ERROR branch — which also stops the attacker-supplied string being logged. The approach matches an existing repo pattern (WorkflowExecutionsResource.scala:685, with the rationale spelled out at ComputingUnitAccessResource.scala:117-119).
Both endpoints that reach the guard are covered by the single line — /dashboard/search and the anonymous /dashboard/publicSearch — and the frontend's only status-sensitive interceptor gates on 401 alone, so nothing downstream changes.
Four concerns were investigated and dismissed with evidence rather than dropped. The one worth naming: @QueryParam("includePublic ") in this same file carries a trailing space, which looks like a live bug and is not — publicSearch passes a null uid and WorkflowSearchQueryBuilder.scala:83 forces IS_PUBLIC = true regardless of that flag.
|
Updates are in and CI is green. Ready to merge when you have a chance. |
0c741f9 to
b3403b3
Compare
|
Amber CI failed in unrelated WorkflowResourceSpec. The dashboard tests passed. Could someone rerun the failed job? |
b3403b3 to
6802e98
Compare
|
I reproduced the CI failure locally. WorkflowResourceSpec still expected IllegalArgumentException after this PR changed it to BadRequestException. Updated that assertion in 6802e98. Both resource suites now pass all 87 tests, and scalafixAll --check and scalafmtCheckAll pass. |
6802e98 to
22a02b8
Compare
22a02b8 to
ad490c9
Compare
|
@Yicong-Huang, the refreshed checks are green. Could you take a final look and merge when ready? The stale fix label should be feature. |
What changes were proposed in this PR?
Return
BadRequestExceptionfor an unknown dashboard resource type instead of an unhandledIllegalArgumentException.Before: invalid resource types return 500.
After: they return 400 with a specific message.
Update the exception assertions in both dashboard test suites.
Any related issues, documentation, discussions?
Closes #8143
How was this PR tested?
Retested in a fresh detached worktree from upstream main at
7426431337, which includes #8938. The regression assertions failed before the implementation change: 3 failed and 83 passed. After the change, all 86 focused tests pass.Used JDK 17 and a disposable PostgreSQL database initialized from
sql/texera_ddl.sql. The tests use embedded PostgreSQL. Object-storage endpoints were isolated for the passing run.Repository lint, formatting and whitespace checks pass.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex