epugh commented on PR #4722:
URL: https://github.com/apache/solr/pull/4722#issuecomment-5749491457
Okay, I've buffed up this PR. One thing is that at the end I asked Codex
to review the nodes threads API against the other V2 Nodes apis, and I got this
feedback that I don't want to lose:
Yes. I compared the current patch with node system info, properties, health,
and logging. The migration follows the
main Jersey conventions, but there are several worthwhile improvements.
This was a source review; I haven’t changed
files or rerun tests.
For this PR, I would prioritize:
1. Use the shared response factory.
solr/core/src/java/org/apache/solr/handler/admin/api/NodeThreadsAPI.java:46
directly constructs
NodeThreadsResponse. The other four implementations use
instantiateJerseyResponse(...), which registers the
response with the request context so exception handling can preserve
partially populated responses. This is a
small consistency fix—not a claim that successful responses currently
fail.
2. Add a generated-client HTTP test.
Our V2 response test
(solr/core/src/test/org/apache/solr/handler/admin/ThreadDumpHandlerTest.java:86)
calls the
resource directly and serializes the result. It doesn’t exercise Jersey
registration, routing, or generated-
client deserialization. Both properties
(solr/core/src/test/org/apache/solr/handler/admin/api/
GetNodePropertiesTest.java:36) and system info
(solr/core/src/test/org/apache/solr/handler/admin/api/
GetNodeSystemInfoTest.java:47) provide good HTTP-test examples. Threads
should have an equivalent using
SystemApi.GetThreadDump.
3. Document the response fields in OpenAPI.
solr/api/src/java/org/apache/solr/client/api/model/NodeThreadsResponse.java:24
has @JsonProperty annotations but
no @Schema descriptions. Particularly useful details are CPU-time
units, optional fields, and whether absent
deadlocks means none were detected. Properties already supplies schema
descriptions; the developer conventions
encourage them.
4. Reuse the existing UI client factory.
ThreadsV2 (solr/webapp/web/js/angular/services.js:211) and SystemV2
construct the same generated SystemApi
client. The threads controller could use SystemV2 directly. Several
comments in that file also still describe
Threads as a plain $resource; those are now stale.
The larger design differences are:
Area Current patterns
Opportunity
━━━━━━━━━━━━━━━━━━━ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Data collection Threads collects in the v1 handler; system
Prefer a shared provider with separate v1 and
info uses SystemInfoProvider; properties/ v2
adapters when their representations differ
health/logging put much of the logic in
Jersey resources
─────────────────── ──────────────────────────────────────────────
───────────────────────────────────────────────
Generated clients Threads/system use the system tag;
Establish a deliberate grouping convention.
properties/health use node; logging uses
Changing existing tags also changes generated
logging
client classes
─────────────────── ──────────────────────────────────────────────
───────────────────────────────────────────────
Response shape Threads retains system.threadDump[].thread A
direct array of thread objects would be
wrappers
simpler, but would require an explicit
compatibility decision
─────────────────── ──────────────────────────────────────────────
───────────────────────────────────────────────
Targeting System info supports nodes proxying; other
Document local versus multi-node behavior
compared APIs are local
consistently; avoid adding proxying
implicitly during migration
─────────────────── ──────────────────────────────────────────────
───────────────────────────────────────────────
Caching System info explicitly disables caching in
Verify HTTP headers across node APIs and
its Jersey method; threads does so only in
establish a common policy for live diagnostic
v1
responses
The provider approach is the strongest architectural opportunity. My
compatibility fix deliberately retained the
original collector to restore v1 safely. Moving that collector into a
shared provider would remove the Jersey
resource’s dependency on ThreadDumpHandler while keeping both response
contracts intact.
What already aligns well: the interface/implementation split,
JerseyResource inheritance, handler-based
registration, SolrJerseyResponse, and preserving the existing
METRICS_READ_PERM. Threads should retain that
permission even though some neighboring APIs use CONFIG_READ_PERM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
