chore(amber): remove the commented-out MockKillWorkerResource - #8396
chore(amber): remove the commented-out MockKillWorkerResource#8396aglinxinyuan wants to merge 1 commit into
Conversation
Automated Reviewer SuggestionsBased on the
|
There was a problem hiding this comment.
🟢 Approval recommended
The change is a straightforward cleanup (dead code/comment removal) and repository-wide search shows no remaining references.
Pull request overview
Removes a long-dead Amber Dropwizard/Jersey endpoint stub (MockKillWorkerResource) that was never actually registered/served, simplifying the web service surface area without changing runtime behavior.
Changes:
- Removed the commented-out Jersey registration line for
MockKillWorkerResourcefromTexeraWebApplication. - Deleted the
MockKillWorkerResource.scalafile (which contained only commented-out code).
File summaries
| File | Description |
|---|---|
| amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala | Removes the leftover commented Jersey registration for the dead resource. |
| amber/src/main/scala/org/apache/texera/web/resource/MockKillWorkerResource.scala | Deletes the unused, never-registered resource stub file. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 545 | 0.333 | 17,093/27,227/27,227 us | 🔴 +20.9% / 🔴 +70.4% |
| 🔴 | bs=100 sw=10 sl=64 | 1,193 | 0.728 | 82,622/116,312/116,312 us | 🔴 +16.4% / 🟢 +22.2% |
| 🟢 | bs=1000 sw=10 sl=64 | 1,418 | 0.866 | 702,938/793,290/793,290 us | 🟢 -6.7% / 🟢 +41.0% |
Baseline details
Latest main 1cbe857 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 545 tuples/sec | 640 tuples/sec | 755.36 tuples/sec | -14.8% | -27.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.333 MB/s | 0.39 MB/s | 0.461 MB/s | -14.6% | -27.8% |
| bs=10 sw=10 sl=64 | p50 | 17,093 us | 14,138 us | 12,938 us | +20.9% | +32.1% |
| bs=10 sw=10 sl=64 | p95 | 27,227 us | 25,017 us | 15,980 us | +8.8% | +70.4% |
| bs=10 sw=10 sl=64 | p99 | 27,227 us | 25,017 us | 19,233 us | +8.8% | +41.6% |
| bs=100 sw=10 sl=64 | throughput | 1,193 tuples/sec | 1,218 tuples/sec | 976.3 tuples/sec | -2.1% | +22.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.728 MB/s | 0.743 MB/s | 0.596 MB/s | -2.0% | +22.2% |
| bs=100 sw=10 sl=64 | p50 | 82,622 us | 79,797 us | 102,340 us | +3.5% | -19.3% |
| bs=100 sw=10 sl=64 | p95 | 116,312 us | 99,915 us | 109,262 us | +16.4% | +6.5% |
| bs=100 sw=10 sl=64 | p99 | 116,312 us | 99,915 us | 118,827 us | +16.4% | -2.1% |
| bs=1000 sw=10 sl=64 | throughput | 1,418 tuples/sec | 1,403 tuples/sec | 1,006 tuples/sec | +1.1% | +40.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.866 MB/s | 0.856 MB/s | 0.614 MB/s | +1.2% | +41.0% |
| bs=1000 sw=10 sl=64 | p50 | 702,938 us | 698,287 us | 999,855 us | +0.7% | -29.7% |
| bs=1000 sw=10 sl=64 | p95 | 793,290 us | 850,512 us | 1,042,833 us | -6.7% | -23.9% |
| bs=1000 sw=10 sl=64 | p99 | 793,290 us | 850,512 us | 1,070,722 us | -6.7% | -25.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,366.79,200,128000,545,0.333,17092.54,27226.66,27226.66
1,100,10,64,20,1675.97,2000,1280000,1193,0.728,82621.86,116312.21,116312.21
2,1000,10,64,20,14100.57,20000,12800000,1418,0.866,702937.89,793290.01,793290.01
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8396 +/- ##
============================================
- Coverage 94.03% 94.02% -0.01%
+ Complexity 4821 4820 -1
============================================
Files 1204 1204
Lines 48991 48991
Branches 5956 5956
============================================
- Hits 46067 46066 -1
Misses 1458 1458
- Partials 1466 1467 +1
*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
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 2 must-fix · 1 advisory · 0 polish: the deletion itself is clean and complete. Both must-fix items are wrong provenance claims that ship as the squash message.
Conventions (2)
TexeraWebApplication.scala:125: the title says "never-registered", git says it was registered for five months (must-fix, see inline)TexeraWebApplication.scala:125: the History table's two attributed commits are both wrong (must-fix, see inline)
Simplifications (1)
ExecutionStatusEnum.scala: license header only, zero references repo-wide, the same shape as the file removed here (advisory, a follow-up rather than scope for this PR)
Verification trace
Every line of the removed file is a comment after the ASF header, so zero compiled code leaves the tree. git grep at head 5219053 finds no surviving reference to the symbol or to the two things its commented body named (KillAndRecover, WorkflowWebsocketResource.sessionJobs), both already gone. No frontend call site, nginx route, or gateway route mentions /kill, and the 22 live environment.jersey.register(...) calls are untouched.
| @@ -125,7 +125,6 @@ class TexeraWebApplication | |||
| environment.servlets.setSessionHandler(new SessionHandler) | |||
There was a problem hiding this comment.
Must fix:
"never-registered" is refuted by git: TexeraWebApplication registered this class live from 41a8a9201 (2020-08-20) until 4a64935c3 (2021-01-30). "commented-out" is both accurate and the stronger claim, since the file defines nothing at all. Suggested: chore(amber): remove the commented-out MockKillWorkerResource. Type and scope are correct and stay.
There was a problem hiding this comment.
Applied — the title is now chore(amber): remove the commented-out MockKillWorkerResource, and the description's opening line says what the file actually is (every non-blank line after the ASF header is a comment) instead of calling it a never-registered resource. Re-checked the live window against the diffs: 41a8a92017 registered it on 2020-08-20 and #1000 (4a64935c35) commented the registration out on 2021-01-30 — about five months registered.
Squash here takes PR_TITLE + PR_BODY (repo settings), so the corrected text is what ships; the branch commit's subject is left as is.
🤖 Addressed by Claude Code
| @@ -125,7 +125,6 @@ class TexeraWebApplication | |||
| environment.servlets.setSessionHandler(new SessionHandler) | |||
There was a problem hiding this comment.
Must fix:
Both commits the History table attributes are wrong. 134bc9ba64 replaced the named import with a wildcard, and environment.jersey().register(classOf[MockKillWorkerResource]) is still an uncommented line in that same diff. #1394 (88ff5c840) only reindented a placeholder that #1000 (4a64935c3) had created. #1000 is the commit that actually ended this code and the table never names it. That also corrects "dead for about six years" to roughly 4.6. Issue #8393 carries the same text.
There was a problem hiding this comment.
Applied — the History table now has two rows, Introduced by 41a8a92017 (2020-08-20, registered live) and Disabled by #1000 (4a64935c35, 2021-01-30), which commented out the registration line and the whole file body in the same diff. 134bc9ba64 and #1394 are out of the table — re-read both diffs, and neither ended the code: the first replaces the named import with edu.uci.ics.texera.web.resource._ while environment.jersey().register(classOf[MockKillWorkerResource]) sits unchanged as a context line, and the second — like #1251 (933b5775e8) before it — only re-indents the already-commented line. Issue #8393's title and body carry the same corrections.
One number I land differently: 2021-01-30 to today is about 5.6 years (five years to 2026-01-30, plus seven months), not 4.6 — so the text now says "about five and a half years". That also costs it the "longest-standing item in this series" line, which is gone from the body: #7451's ElidableStatement lost its last caller on 2021-01-11, and #8333's PropertyNameConstants was never adopted after 2020-08-20.
🤖 Addressed by Claude Code
aglinxinyuan
changed the title
|
On the 🤖 Addressed by Claude Code |
What changes were proposed in this PR?
Deletes
MockKillWorkerResource.scala, a file whose every non-blank line after the ASF header is a comment — the commented-out remains of a JAX-RS/kill/workerendpoint — together with the commented-out Jersey registration line inTexeraWebApplicationthat pointed at it. Pure deletion, no behaviour change: −40 lines, none of them compiled.History
41a8a92017(2020-08-20) — predates the PR workflow — which also registered it live with Jersey4a64935c35, 2021-01-30) — "Migrate all control handling logics to AsyncRPC and cleanup Amber" commented out the registration line and the whole file body in the same diff. Nothing has compiled from the file sinceRegistered live for about five months, 2020-08-20 → 2021-01-30, then commented out for the five and a half years since. The commented registration line goes with the file.
Any related issues, documentation, discussions?
Closes #8393
How was this PR tested?
Existing tests only — no spec referenced it.
Locally, from the repo root with Java 17:
sbt "WorkflowExecutionService/Test/compile"— success.sbt scalafmtCheckAll "scalafixAll --check"— clean.Verification, re-runnable by a reviewer:
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5, Claude Fable 5.1)