gerlowskija commented on code in PR #4078: URL: https://github.com/apache/solr/pull/4078#discussion_r2868903943
########## solr/api/src/java/org/apache/solr/client/api/endpoint/NodeSystemInfoApi.java: ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.api.endpoint; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.Parameter; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import jakarta.ws.rs.QueryParam; +import org.apache.solr.client.api.model.NodeSystemResponse; +import org.apache.solr.client.api.util.Constants; + +/** V2 API definitions to fetch node system info, analogous to the v1 /admin/info/system. */ +@Path(Constants.NODE_INFO_SYSTEM_PATH) +public interface NodeSystemInfoApi { + + @GET + @Operation( + 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( Review Comment: [Q] Is this endpoint a replacement of the one on L36 above? Or is there information returned by 'getNodeSystemInfo' that isn't returned this method? [Q] Might a user want to specify multiple of the 'gpu', 'jvm', 'lucene', etc. values? Say, maybe a user is writing their own UI and wants to display both the Java and Lucene versions to readers. I guess I'm wondering whether we should be separating things out at this level. Or at least - maybe we should have this be a query-param rather than a path-param so users can specify multiple values in a single request. ########## solr/solrj/src/java/org/apache/solr/client/solrj/request/SystemInfoV2Request.java: ########## @@ -0,0 +1,74 @@ +/* Review Comment: [-1] The JAX-RS annotations in `NodeSystemInfoApi` will be put into an "OpenAPI Spec" ("OAS"). Our build uses that spec to generate a bunch of SolrJ request and response objects. That's all to say - we don't need to implement this file ourselves. Something like it will appear in the source tree as soon as you run `./gradlew solr:solrj:openApiGenerate`. That command should create a file called `SystemApi` which should be roughly equivalent to this file here. With that in mind - let's delete this file. ########## solr/solrj/src/test/org/apache/solr/client/solrj/response/SystemInfoV2ResponseTest.java: ########## @@ -0,0 +1,103 @@ +/* Review Comment: [0] If we're able to delete `SystemInfoV2Request` and `SystemInfoV2Response` as I suggest elsewhere, then this test could probably be switched over to using the auto-generated versions of those classes instead? ########## solr/api/src/java/org/apache/solr/client/api/util/Constants.java: ########## @@ -47,4 +47,6 @@ private Constants() { public static final String ADDTL_FIELDS_PROPERTY = "hasAdditionalFields"; public static final String JAVABIN_CONTENT_TYPE_V2 = "application/vnd.apache.solr.javabin"; + + public static final String NODE_INFO_SYSTEM_PATH = "/node/info/system"; Review Comment: [-0] Unless there's a particular reason to use the new path (i.e. '/node/info/system'), then I'd opt for sticking with '/node/system' and deleting the existing "@Endpoint" bit. ########## solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoV2Response.java: ########## @@ -0,0 +1,50 @@ +/* Review Comment: [-1] See my comment on SystemInfoV2Request. If the JAX-RS definition is set up correctly Solr's build will auto-generate request and response SolrJ classes, so we shouldn't need those classes in version control and this file should probably be deleted. ########## changelog/unreleased/PR#4078-node-system-response-v2-jersey-resource.yml: ########## @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Node System Info V2 Jersey Resource Review Comment: [0] These changelog files eventually get combined with one another in different ways to create a release's Upgrade Notes, CHANGELOG.md, etc. So we want the "title" here to be as meaningful as possible for a user that's considering an upgrade. Maybe something like: > Add v2 coverage for "System Info" API, and accompanying SolrJ request class `SystemApi.GetNodeSystemInfo`. Or maybe it'd be better with the SolrJ change front and center since that's what's truly "new" to a user... > SolrJ now offers a binding for the Solr "System Info" API, available at `SystemApi.getNodeSystemInfo`. ########## solr/core/src/java/org/apache/solr/handler/admin/api/NodeSystemInfoAPI.java: ########## @@ -32,8 +35,14 @@ * version, etc.), and JVM settings. * * <p>This API (GET /v2/node/system) is analogous to the v1 /admin/info/system. + * + * @deprecated Use the JAX-RS API: NodeSystemInfo (/node/info/system), implementing NodeSystemApi, + * and returning NodeSystemResponse. */ +@Deprecated Review Comment: Yep, please do! -- 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]
