gerlowskija commented on code in PR #4240: URL: https://github.com/apache/solr/pull/4240#discussion_r3395096323
########## changelog/unreleased/SOLR-18161-improve-system-info-response.yml: ########## @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Improve /node/system response with a wrapper to separate the system info from the response header, and add a method to return only selected info Review Comment: ```suggestion title: Added new `/api/node/system` sub APIs to fetch specific subsets of the normal `/api/node/system` response. e.g. `/api/node/system/jvm` ``` The new API endpoints are the most relevant bit to a user, so might be better to focus mainly on those in the changelog entry. ########## gradle/libs.versions.toml: ########## @@ -240,8 +240,8 @@ amazon-awssdk-s3 = { module = "software.amazon.awssdk:s3", version.ref = "amazon amazon-awssdk-sdkcore = { module = "software.amazon.awssdk:sdk-core", version.ref = "amazon-awssdk" } amazon-awssdk-sts = { module = "software.amazon.awssdk:sts", version.ref = "amazon-awssdk" } androidx-lifecycle-runtimeCompose = { module = "org.jetbrains.androidx.lifecycle:lifecycle-runtime-compose", version.ref = "androidx-lifecycle" } -androidx-lifecycle-viewmodelCompose = { module = "org.jetbrains.androidx.lifecycle:lifecycle-viewmodel-compose", version.ref = "androidx-lifecycle" } androidx-lifecycle-viewModelNav3 = { module = "org.jetbrains.androidx.lifecycle:lifecycle-viewmodel-navigation3", version.ref = "androidx-lifecycle" } +androidx-lifecycle-viewmodelCompose = { module = "org.jetbrains.androidx.lifecycle:lifecycle-viewmodel-compose", version.ref = "androidx-lifecycle" } Review Comment: Hmm, wonder why this changed 🤔 Oh well 🤷 ########## changelog/unreleased/SOLR-18161-improve-system-info-response.yml: ########## @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Improve /node/system response with a wrapper to separate the system info from the response header, and add a method to return only selected info +type: changed # added, changed, fixed, deprecated, removed, dependency_update, security, other +authors: + - name: Isabelle Giguère +links: + - name: SOLR-18161 + url: https://issues.apache.org/jira/browse/SOLR-18161 Review Comment: ```suggestion url: https://issues.apache.org/jira/browse/SOLR-18161 - name: Github PR url: https://github.com/apache/solr/pull/4240 ``` ########## solr/core/src/java/org/apache/solr/handler/admin/api/GetNodeSystemInfo.java: ########## @@ -62,7 +62,41 @@ public NodeSystemResponse getNodeSystemInfo(String nodes) { SystemInfoProvider provider = new SystemInfoProvider(solrQueryRequest); provider.getNodeSystemInfo(response); if (log.isTraceEnabled()) { - log.trace("Node {}, core root: {}", response.node, response.coreRoot); + log.trace("Node {}, core root: {}", response.nodeInfo.node, response.nodeInfo.coreRoot); + } + return response; + } + + @Override + public NodeSystemResponse getSpecificNodeSystemInfo(String requestedInfo, String nodes) { + NodeSystemResponse response = instantiateJerseyResponse(NodeSystemResponse.class); + solrQueryResponse.setHttpCaching(false); + Review Comment: [0] IMO we should sanity-check "requestedInfo" value here before (potentially) proxying things out to other nodes. I'd have to test it but I imagine the error back to a user will probably turn out much nicer that way in the multi-node case ########## solr/core/src/test/org/apache/solr/handler/admin/api/GetNodeSystemInfoTest.java: ########## @@ -52,7 +52,23 @@ public void testGetNodeInfoHttp() throws Exception { // Test a few values, majority of field-level validation occurs in NodeSystemInfoProviderTest assertEquals(0, infoRsp.responseHeader.status); - assertEquals("std", infoRsp.mode); - assertEquals(Version.LATEST.toString(), infoRsp.lucene.luceneSpecVersion); + assertEquals("std", infoRsp.nodeInfo.mode); + assertEquals(Version.LATEST.toString(), infoRsp.nodeInfo.lucene.luceneSpecVersion); + } + + @Test + public void testGetSpecificNodeInfo() throws Exception { + final var req = new SystemApi.GetSpecificNodeSystemInfo("lucene"); Review Comment: [+1] Love to see the generated SolrJ class getting used here - 🎉 ########## changelog/unreleased/SOLR-18161-improve-system-info-response.yml: ########## @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Improve /node/system response with a wrapper to separate the system info from the response header, and add a method to return only selected info +type: changed # added, changed, fixed, deprecated, removed, dependency_update, security, other +authors: + - name: Isabelle Giguère +links: + - name: SOLR-18161 + url: https://issues.apache.org/jira/browse/SOLR-18161 Review Comment: Not sure I got the indentation on that quite right in Github, but hopefully you get the idea. ########## solr/api/src/java/org/apache/solr/client/api/endpoint/NodeSystemInfoApi.java: ########## @@ -31,4 +33,18 @@ public interface NodeSystemInfoApi { summary = "Retrieve all node system info.", tags = {"system"}) NodeSystemResponse getNodeSystemInfo(@QueryParam(value = "nodes") String nodes); + + @GET + @Operation( + summary = "Retrieve specific node system info.", + tags = {"system"}, + parameters = { + @Parameter( + name = "requestedInfo", + description = "Allowed values: 'gpu', 'jvm', 'lucene', 'security', 'system'") + }) + @Path("/{requestedInfo}") + NodeSystemResponse getSpecificNodeSystemInfo( + @PathParam(value = "requestedInfo") String requestedInfo, Review Comment: [0] I wonder if we could change this from a "String" to some type of enum so that Jersey would do the value-checking for us... ########## solr/solr-ref-guide/modules/configuration-guide/pages/system-info-handler.adoc: ########## @@ -293,3 +309,37 @@ curl http://localhost:8983/api/node/system } } ---- + + +=== Retrieve specific system information. Review Comment: [+1] Excellent, thanks! ########## solr/core/src/test/org/apache/solr/handler/admin/api/GetNodeSystemInfoTest.java: ########## @@ -52,7 +52,23 @@ public void testGetNodeInfoHttp() throws Exception { // Test a few values, majority of field-level validation occurs in NodeSystemInfoProviderTest assertEquals(0, infoRsp.responseHeader.status); - assertEquals("std", infoRsp.mode); - assertEquals(Version.LATEST.toString(), infoRsp.lucene.luceneSpecVersion); + assertEquals("std", infoRsp.nodeInfo.mode); + assertEquals(Version.LATEST.toString(), infoRsp.nodeInfo.lucene.luceneSpecVersion); + } + + @Test + public void testGetSpecificNodeInfo() throws Exception { + final var req = new SystemApi.GetSpecificNodeSystemInfo("lucene"); + + final var infoRsp = req.process(solrTestRule.getSolrClient(null)); + + // Test a few values, majority of field-level validation occurs in NodeSystemInfoProviderTest + assertEquals(0, infoRsp.responseHeader.status); + assertNotNull(infoRsp.nodeInfo.lucene); + assertEquals(Version.LATEST.toString(), infoRsp.nodeInfo.lucene.luceneSpecVersion); + assertNull(infoRsp.nodeInfo.jvm); Review Comment: [0] From an OAS or API documentation perspective, it might be nice at some point to have separate methods and response classes for (e.g.) `/api/node/system/lucene` so that the response object doesn't even have fields for things that'll never be set like `.nodeInfo.gpu`. But I'm happy to punt that down the road unless you think it's worth tackling here? ########## solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java: ########## @@ -82,11 +80,6 @@ public Collection<Class<? extends JerseyResource>> getJerseyResources() { return Set.of(GetNodeSystemInfo.class); } - @Override Review Comment: [Q] Hmm, I didn't expect this to go away. Remind me: is this redundant wit ha parent class definition, or is it one of the markers we only care about for the "old-style" (i.e. non JAX-RS) APIs? ########## solr/solr-ref-guide/modules/configuration-guide/pages/system-info-handler.adoc: ########## @@ -46,6 +44,9 @@ http://localhost:8983/api/node/system == Response + +== Node Information Object Review Comment: [-0] Documenting this is kindof tricky, since the "nodeInfo" object exists for v2 responses, but not v1 responses. Maybe it's worth putting a sentence right under this header indicating that this sub-object is only present in the v2 API response, with fields being present on the top level response in the v1 case. ########## solr/solrj/src/java/org/apache/solr/client/solrj/response/json/JacksonDataBindResponseParser.java: ########## @@ -82,4 +82,29 @@ public NamedList<Object> processResponse(InputStream stream, String encoding) th return result; } + Review Comment: [Q/-0] Hmm, can you explain why this is here? At a glance it doesn't seem related to the rest of the PR but maybe I'm forgetting some context. -- 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]
