refactor(resource): deduplicate HealthCheckResource across services#5979
refactor(resource): deduplicate HealthCheckResource across services#5979Ma77Ball wants to merge 5 commits into
Conversation
HealthCheckResource.scala is duplicated amongst multiple services this pr creates a shared resource to avoid code duplication
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5979 +/- ##
============================================
+ Coverage 56.23% 56.47% +0.24%
- Complexity 2987 2999 +12
============================================
Files 1120 1126 +6
Lines 43167 43699 +532
Branches 4658 4734 +76
============================================
+ Hits 24274 24679 +405
- Misses 17472 17563 +91
- Partials 1421 1457 +36
*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:
|
|
ah @Ma77Ball I think health check does not belong to auth module. I understand you want to have a common place for that, but not in auth.. |
Yicong-Huang
left a comment
There was a problem hiding this comment.
if needed, you can add common/resource project to contain this part of code.
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 376 | 0.23 | 26,325/32,018/32,018 us | 🔴 +12.5% / 🔴 +112.5% |
| 🔴 | bs=100 sw=10 sl=64 | 785 | 0.479 | 125,203/146,663/146,663 us | 🔴 +7.2% / 🔴 +34.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 907 | 0.554 | 1,102,489/1,139,262/1,139,262 us | ⚪ within ±5% / 🔴 +10.0% |
Baseline details
Latest main 0e0ec11 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 376 tuples/sec | 402 tuples/sec | 770.82 tuples/sec | -6.5% | -51.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.23 MB/s | 0.246 MB/s | 0.47 MB/s | -6.5% | -51.1% |
| bs=10 sw=10 sl=64 | p50 | 26,325 us | 23,400 us | 12,723 us | +12.5% | +106.9% |
| bs=10 sw=10 sl=64 | p95 | 32,018 us | 34,083 us | 15,070 us | -6.1% | +112.5% |
| bs=10 sw=10 sl=64 | p99 | 32,018 us | 34,083 us | 18,429 us | -6.1% | +73.7% |
| bs=100 sw=10 sl=64 | throughput | 785 tuples/sec | 822 tuples/sec | 973.75 tuples/sec | -4.5% | -19.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.479 MB/s | 0.502 MB/s | 0.594 MB/s | -4.6% | -19.4% |
| bs=100 sw=10 sl=64 | p50 | 125,203 us | 121,481 us | 102,519 us | +3.1% | +22.1% |
| bs=100 sw=10 sl=64 | p95 | 146,663 us | 136,833 us | 108,855 us | +7.2% | +34.7% |
| bs=100 sw=10 sl=64 | p99 | 146,663 us | 136,833 us | 117,788 us | +7.2% | +24.5% |
| bs=1000 sw=10 sl=64 | throughput | 907 tuples/sec | 920 tuples/sec | 1,004 tuples/sec | -1.4% | -9.7% |
| bs=1000 sw=10 sl=64 | MB/s | 0.554 MB/s | 0.562 MB/s | 0.613 MB/s | -1.4% | -9.6% |
| bs=1000 sw=10 sl=64 | p50 | 1,102,489 us | 1,092,596 us | 1,001,930 us | +0.9% | +10.0% |
| bs=1000 sw=10 sl=64 | p95 | 1,139,262 us | 1,123,368 us | 1,042,923 us | +1.4% | +9.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,139,262 us | 1,123,368 us | 1,074,893 us | +1.4% | +6.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,531.83,200,128000,376,0.230,26325.30,32017.55,32017.55
1,100,10,64,20,2549.15,2000,1280000,785,0.479,125202.76,146663.09,146663.09
2,1000,10,64,20,22039.34,20000,12800000,907,0.554,1102489.17,1139262.21,1139262.21|
Moved |
|
/request-review @Yicong-Huang |
|
this has nothing to do with ASF right? |
added HealthCheckResourceSpec to test that the health check resource is working and reports to codecov to get coverage results for this on github
What changes were proposed in this PR?
HealthCheckResourcein thecommon/authmodule under the existing packageorg.apache.texera.service.resource, which all six backend services already depend on.register(classOf[HealthCheckResource])call, and test spec works unchanged with zero call-site edits.org.apache.texera.web.resource.HealthCheckResourceis intentionally left out of scope: it uses the olderjavax.ws.rsnamespace and lacks@PermitAll, so it is not safely mergeable.Any related issues, documentation, discussions?
Closes: #5976
How was this PR tested?
*RunSpecsuites already assertclassOf[HealthCheckResource]registration and continue to pass against the shared class.sbt "Auth/compile" "ConfigService/compile" "AccessControlService/compile" "FileService/compile" "ComputingUnitManagingService/compile" "WorkflowCompilingService/compile" "NotebookMigrationService/compile", expect all to succeed (they did locally).curl localhost:<port>/api/healthcheck, expect{"status":"ok"}.Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8 in compliance with ASF