gerlowskija commented on code in PR #4722:
URL: https://github.com/apache/solr/pull/4722#discussion_r3829839454
##########
changelog/v9.10.0/SOLR-16458.yml:
##########
@@ -0,0 +1,7 @@
+title: Add JAX-RS API for retrieving node thread information.
Review Comment:
[0] These changelog entries eventually get converted to user-facing
documentation, so it's important to emphasize the aspects of the change that
they're most likely to run into or care about.
JAX-RS is probably an implementation detail from a user's perspective.
(Unless you can think of a reason a Solr user might be interested that I'm
missing?)
What they're more likely to care about is that because this API is now
JAX-RS it gets included in our OpenAPI specification, and therefore gets
coverage in the Python, JS, and Java clients that we generate from that spec.
With that in mind, I'd suggest a title like:
```suggestion
title: SolrJ now offers a SolrRequest class allowing users fetch single-node
thread-dumps: NodeApi.ThreadDump
```
(I'm not sure if `NodeApi.ThreadDump` is the actual name of the resulting
SolrJ class; need to double-check that bit...)
##########
changelog/v9.10.0/SOLR-16458.yml:
##########
Review Comment:
[-1] Our convention is that changelog entries are initially put into
`changelog/unreleased/` when initially written, and then moved later on into a
release-specific directory by a Release Manager ("RM").
Can you please move this to `changelog/unreleased/`?
##########
solr/api/src/java/org/apache/solr/client/api/endpoint/NodeThreadsApi.java:
##########
@@ -0,0 +1,12 @@
+package org.apache.solr.client.api.endpoint;
+
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.Path;
+import org.apache.solr.client.api.model.NodeThreadsResponse;
+
+@Path("/node/threads")
+public interface NodeThreadsApi {
Review Comment:
[Q] This interface looks like a good API definition, but does anything
actually implement it?
Generally in our JAX-RS setup, we define an interface containing the JAX-RS
annotations (done! Good work!), and then there's a class over in solr-core that
implements the interface and has the actual functionality.
See our [developer
documentation](https://github.com/apache/solr/blob/main/dev-docs/apis.adoc#writing-jax-rs-apis)
for a writeup of how these JAX-RS APIs are expected to work.
In this case I kindof expected `NodeThreadsAPI` to get a new `implements
NodeThreadsApi` clause, and then for the existing "triggerThreadDump" method in
that class to be reworked to fit the `NodeThreadsResponse getThreadDump()`
method definition here.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/NodeThreadsAPI.java:
##########
@@ -41,7 +36,8 @@ public NodeThreadsAPI(ThreadDumpHandler handler) {
path = {"/node/threads"},
method = GET,
permission = METRICS_READ_PERM)
- public void triggerThreadDump(SolrQueryRequest req, SolrQueryResponse rsp)
throws Exception {
+ public void triggerThreadDump(
Review Comment:
[0] This method needs tweaked so that it uses the `NodeThreadsResponse
getThreadDump()` signature.
##########
solr/core/src/java/org/apache/solr/handler/admin/ThreadDumpHandler.java:
##########
@@ -179,12 +178,6 @@ public Category getCategory() {
public Collection<Api> getApis() {
return AnnotatedApi.getApis(new NodeThreadsAPI(this));
}
-
- @Override
Review Comment:
[-0] Generally how we've been registering these v2 API classes once they're
converted to JAX-RS is that:
1. `registerV2` still exists and returns true for the RequestHandler
2. The v2 API name is removed from `getApis()` (this method is for our
non-JAX-RS APIs)
3. The v2 API name is added to a method called `getJerseyResources()`. (See
`CollectionsHandler` or an existing RequestHandler for a good example what this
looks like.)
--
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]
