Skip to content

SOLR-16458: Node system info V2 api jersey resource#4078

Open
igiguere wants to merge 30 commits intoapache:mainfrom
igiguere:NodeSystemInfoApi-JerseyResource
Open

SOLR-16458: Node system info V2 api jersey resource#4078
igiguere wants to merge 30 commits intoapache:mainfrom
igiguere:NodeSystemInfoApi-JerseyResource

Conversation

@igiguere
Copy link
Contributor

@igiguere igiguere commented Jan 26, 2026

https://issues.apache.org/jira/browse/SOLR-16458

Jira Ticket

The Excel spreadsheet links to SOLR-16458 for the V1 /solr/admin/info/system and V2 /api/node/system, although the ticket does not mention those URLs.

From the checklist below : "I have created a Jira issue and added the issue ID to my pull request title." - m'well, no. But keeping track of these V2 tickets is probably difficult enough without adding one more.

Sorry about the mix-up in the commit messages. I originally landed on the wrong ticket.

Description

Implementation of a Jersey resource to support getting the system info. This new resource should replace the home-brew "Endpoint" V2 resource.

QUESTIONS:

  • AdminHandlersProxy does not support V2, so this PR does not test parameter "nodes". Ref: PR SOLR-16738: Refactor special-casing out of AdminHandlersProxy  #3991, mentioned in PR SOLR-17436: Create a v2 equivalent for /admin/metrics #4057
  • TO BE HANDLED IN A FOLLOW_UP PR: I don't like that the system info returned in the single-node response is not separate from the response header (i.e.: fields "node"(name), "mode", "core_root"... at the same level as "responseHeader").
    Suggestion: wrap the V2 system info in "nodeInfo":
    <response>
    <lst name="responseHeader"><!-- ... --></lst>
    <lst name="nodeInfo">
    <str name="node">localhost:8983_solr</str>
    <!-- ... -->
    </lst>
    </response>
    Ideally, IMHO, the "node" field should be named "name" (i.e.: the node name), and then the wrapper "nodeInfo" could be just "node"
  • AdminHandlersProxy wraps all proxied responses into a NamedList where the name is the node name (host:port_solr). This creates a structure with unpredictable (or not easily predictable) keys. If we do adopt a response with the "nodeInfo" wrapper, then, the proxied responses could be added to the list of "nodeInfo"... In a specific implementation of AdminHandlersProxy (ref.SOLR-16738: Refactor special-casing out of AdminHandlersProxy  #3991). This is all a lot of changes, breaking changes for whoever is parsing the response already. Opinions?
  • TODO: Clean-up documentation. Path /admin/info/system is mentionned in 5 pages (last I checked)
  • I have removed the core info from the NodeSystemResponse in this PR, following the recent commit wrt CoreInfoHandler (PR SOLR-18091: Separate out core specific info into CoreInfoHandler #4084). I thought about adding/adapting a V2 resource and response for the core info... but, looking at CoreApis and CoreStatus, there's not much in CoreInfoHandler that is not coverred already in CoreStatus: "now", "dirImpl", and "cwd" (user dir). @epugh : you introduced CoreInfoHandler. What did you have in mind for V2 (Jax-RS, of course)?
    I experimented with creating a V2 endpoint equivalent to the new CoreInfoHandler, but the request's SolrCore is always null. I tried creating a V2 resource like /node/{coreName}/info, and also tried /cores/{coreName}/info. No luck. Or I'm missing something. That code is not included, though.

Solution

Add NodeSystemInfoApi (in solr-api), implemented in GetNodeSystemInfo.

Class SystemInfoProvider contains code to provide the system info, copied from SystemInfoHandler.

Clean-up SystemInfoHandler to use SystemInfoProvider, making sure the response is back-compatible

Tests

Add unit tests for SystemInfoProvider (note that the test class for SystemInfoHandler was actually only testing a method now found in SystemInfoProvider).

Add unit tests for GetNodeSystemInfo.

Functional tests on a local instance (dev-slim, started with the "cloud" example)

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide
  • I have added a changelog entry for my change

Isabelle Giguere and others added 7 commits January 15, 2026 10:17
Add NodeSystemIfoApi (notice the lower-case last chars), to (eventually)
replace Endpoint NodeSystemIfoAPI.  Lo warning about deprecation.

Add GetNodeSystemInfo, move the info-gathering logic to a separate class
NodeSystemInfoProvider, adjust SystemInfoHandler.

Known Issue: NodeSystemInfoResponse does not support responses from
multiple nodes.

More testing and clean-up to do.
Fix NodeSystemResponse to contain a map of node name to node info.

Fix responses for Json and XML.

TODO : nodes=all is ignored
forgot those in the previous commit
Fix log calls.  Add unit tests for HTTP call.
Isabelle Giguere and others added 4 commits January 27, 2026 15:31
Remove the Map from NodeSystemInfoResponse: the AdminHandlersProxy wraps
all nodes responses into a map, it should not belong to the response
class.
Replace the Map wrapper by just a single object wrapper: NodeSystemInfo.
It provides some separation between the response header and the actual
node info.
@gerlowskija
Copy link
Contributor

First off, thanks again for sharing this PR and tackling another v2 API!

Should we keep the URL path /node/system [...] This PR wrap the system info in "nodeInfo" [...] the "node" field should be named "name" [...] a lot of changes, breaking changes

I agree with some of your concerns about the "cosmetics" of the request and API-response as they are today. We're free to change v2 APIs and their responses as much as we'd like. But v1 APIs are required to be backwards-compatible: breaking changes can only occur on major-version upgrades. That's not to say that v1 can't be changed, just that if v1 has a "breaking change" then it can only go to 'main', not branch_10x. So we've got some options depending on how much we care about updating v1 vs. v2-only.

What we've done in a few places elsewhere is make whatever improvements we want to the v2 response format, and then if the v1 codepath calls v2 code have the v1 code do some manual massaging to reshape the response into something that meets backwards compatibility. It's not pretty, but it meets the requirements and is code that will eventually go away when we are able to deprecate and remove some of these v1 endpoints. So that's probably what I'd recommend for these sort of smaller response-changes.

I'm somewhat tempted, as I read your comments, to suggest overhauling this endpoint entirely and making it look radically different in our v2 API. Something like splitting it up into a few smaller paths, like /node/jvm, /node/resources, /node/version, etc. But it's probably better to get things ported over to v2 in the current format and then reevaluate? Idk - curious if you have any thoughts on that.

@igiguere
Copy link
Contributor Author

igiguere commented Jan 30, 2026

First off, thanks again for sharing this PR and tackling another v2 API!

Should we keep the URL path /node/system [...] This PR wrap the system info in "nodeInfo" [...] the "node" field should be named "name" [...] a lot of changes, breaking changes

I agree with some of your concerns about the "cosmetics" of the request and API-response as they are today. We're free to change v2 APIs and their responses as much as we'd like. But v1 APIs are required to be backwards-compatible: breaking changes can only occur on major-version upgrades. That's not to say that v1 can't be changed, just that if v1 has a "breaking change" then it can only go to 'main', not branch_10x. So we've got some options depending on how much we care about updating v1 vs. v2-only.

What we've done in a few places elsewhere is make whatever improvements we want to the v2 response format, and then if the v1 codepath calls v2 code have the v1 code do some manual massaging to reshape the response into something that meets backwards compatibility. It's not pretty, but it meets the requirements and is code that will eventually go away when we are able to deprecate and remove some of these v1 endpoints. So that's probably what I'd recommend for these sort of smaller response-changes.

I'm somewhat tempted, as I read your comments, to suggest overhauling this endpoint entirely and making it look radically different in our v2 API. Something like splitting it up into a few smaller paths, like /node/jvm, /node/resources, /node/version, etc. But it's probably better to get things ported over to v2 in the current format and then reevaluate? Idk - curious if you have any thoughts on that.

Thanks for the feedback, @gerlowskija
I will make sure that the V1 response is back-compatible.

As for splitting the new V2 response into smaller paths, it could be a path parameter, at least for "blocks" of info, like "jvm", "gpu", what else? I'm not sure what /node/version would cover, since there's the Solr version, Lucene version, JVM version. So that might be a query param, like a filter, to get the version of everything that has a version. Unless the param "version" would return the "lucene" block of info?

Isabelle Giguere added 2 commits January 30, 2026 18:12
Ensure back-compatible response from V1 path (and from old V2 path)
Add method getSpecificNodeSystemInfo, with path parameter, to get only
selected info.

Add query param "nodes": not used, because AdminHandlersProxy does not
support V2 yet.
Isabelle Giguere added 3 commits February 10, 2026 16:08
Merge remote-tracking branch 'origin-solr/main' into
NodeSystemInfoApi-JerseyResource
Merge remote-tracking branch 'origin-solr/main' into
NodeSystemInfoApi-JerseyResource
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 10, 2026
Isabelle Giguere and others added 7 commits February 10, 2026 20:35
Commented-out for now.

Revert to class name NodeSystemResponse
Split core info response into a different model class.

Place the getCoreInfo method in the utility class, to re-use in
back-compatible /admin/info/system response.

Rename a few classes

Adjust documentation
…guere/solr.git into NodeSystemInfoApi-JerseyResource
@igiguere igiguere marked this pull request as ready for review February 17, 2026 22:49
Isabelle Giguere and others added 2 commits February 17, 2026 17:56
* @deprecated Use the JAX-RS API: NodeSystemInfo (/node/info/system), implementing NodeSystemApi,
* and returning NodeSystemResponse.
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of deprecating, can we just remove this? We made the V2 api's Experimental because we knew they would be changing. If I understand correctly, this is the "V2 style using custom annotations" approach, which we are phasing out in favour of the "V2 style using Jax-rs". Assuming your new code covers what this did, I think we can just remove it. No need to deprecate. @gerlowskija ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that was my first question, in the description. I can delete the class, no problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, please do!

/**
* @param path the HTTP path to use for this request. Supports V1 "/admin/info/system" (default)
* or V2 "/node/system"
* or old V2 "/node/system"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we just remove this /node/system? We struggle with enoguh tech debt and multiple paths, so I'd love to not have two flavours of this!


/** Request to "/node/info/system" by default, without params. */
public SystemInfoV2Request() {
// TODO: support V2 by default. Requires refactoring throughout the CLI tools, at least
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to enhancing the Solr CLI!

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like great progress. I'd love to eliminate the old V2 eqivalents to simplify our documentation needs.

I think we do need ref guide updates for this?

@epugh
Copy link
Contributor

epugh commented Feb 28, 2026

Also, I think this may have some overlap? #4171

@igiguere
Copy link
Contributor Author

Also, I think this may have some overlap? #4171

I am more worried about #4084 (merged). The new node system reponse will not return the core info, and #4084 does not include support for V2 ... Please refer to the last of the questions in the description.

@igiguere
Copy link
Contributor Author

Looks like great progress. I'd love to eliminate the old V2 eqivalents to simplify our documentation needs.

I think we do need ref guide updates for this?

I did update system-info-handler.adoc. But I there's probably still a few more references to /admin/info/system here and there in the documentation, to be reviewed once the implementation is settled.

Copy link
Contributor

@gerlowskija gerlowskija left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a few things that very likely need changed - left comments about those inline. That should help slim things down a bit.

In general - I'm sympathetic to the cosmetic changes you've considered in this PR but I'm having trouble reviewing the PR in detail. I just don't know our "system info" stuff to tell at a glance what's "new" and what's a refactor of the existing code.

@igiguere would you object to our making this "just" a verbatim JAX-RS migration, and we could handle any cosmetic changes in a subsequent PR? (You could do this, if willing. Or if not, I'm happy to make the changes myself if you don't mind. I've waffled a bit on this already and don't want that to cause you additional work. Just give me a thumbs up or thumbs down and I can get started)

In practice this would mean:

  • deleting the existing class NodeSystemInfoAPI
  • Modifying NodeSystemInfoApi to use the "/node/system" path (at least, for now)
  • Deleting NodeSystemInfoApi.getSpecificNodeSystemInfo (at least, for now)
  • removing the NodeSystemInfo wrapper-object in NodeSystemResponse (again - just for now. Of all your cosmetic improvements this is the one that I like the most! but it adds a ton to the diff and it'd be easier to review in a separate PR)

@@ -0,0 +1,8 @@
# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
title: Node System Info V2 Jersey Resource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird to me to mention a generated class in the changelog, but I'll mention it.

description = "Allowed values: 'gpu', 'jvm', 'lucene', 'security', 'system'")
})
@Path("/{requestedInfo}")
NodeSystemResponse getSpecificNodeSystemInfo(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is removed in the latest commit, following your request to simplify the PR. To be discussed in a follow-up improvement.


public static final String JAVABIN_CONTENT_TYPE_V2 = "application/vnd.apache.solr.javabin";

public static final String NODE_INFO_SYSTEM_PATH = "/node/info/system";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[-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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in the latest commit.

* @deprecated Use the JAX-RS API: NodeSystemInfo (/node/info/system), implementing NodeSystemApi,
* and returning NodeSystemResponse.
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, please do!

@@ -0,0 +1,74 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[-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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in the latest commit.
I had implemented this class, originally, because the existence of both 1) /node/system old V2 with original response structure and 2) /node/info/system new V2 and wrapped response was causing trouble, although, at this time, I can't remember exactly what. I guess I'll see the issue again if we decide to improve the response in a follow-up.

@@ -0,0 +1,103 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

Copy link
Contributor Author

@igiguere igiguere Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class deleted in the latest commit

@epugh
Copy link
Contributor

epugh commented Mar 1, 2026

Thanks @gerlowskija for taking the time to go through, I think your suggestions make a lot of sense! You know this code base better than I! I think verbatim conversion in one PR combined with a second improvement PR makes a lot of sense. In my poking around, there are a lot of places we can improve the V2 apis, and that's why they are labeled Experimental! SO we can change them.

@igiguere
Copy link
Contributor Author

igiguere commented Mar 1, 2026

Found a few things that very likely need changed - left comments about those inline. That should help slim things down a bit.

In general - I'm sympathetic to the cosmetic changes you've considered in this PR but I'm having trouble reviewing the PR in detail. I just don't know our "system info" stuff to tell at a glance what's "new" and what's a refactor of the existing code.

@igiguere would you object to our making this "just" a verbatim JAX-RS migration, and we could handle any cosmetic changes in a subsequent PR? (You could do this, if willing. Or if not, I'm happy to make the changes myself if you don't mind. I've waffled a bit on this already and don't want that to cause you additional work. Just give me a thumbs up or thumbs down and I can get started)

In practice this would mean:

* deleting the existing class NodeSystemInfoAPI

* Modifying NodeSystemInfoApi to use the "/node/system" path (at least, for now)

* Deleting `NodeSystemInfoApi.getSpecificNodeSystemInfo` (at least, for now)

* removing the `NodeSystemInfo` wrapper-object in `NodeSystemResponse` (again - just for now.  Of all your cosmetic improvements this is the one that I like the most! but it adds a ton to the diff and it'd be easier to review in a separate PR)

Hemmm... right. I can scale back. I'll keep the current changes in a stash locally.

Revert changes to the response structure: simpler PR for simpler review.

Remove the old '@endpoint' API implementation and related unit tests.
@igiguere
Copy link
Contributor Author

igiguere commented Mar 2, 2026

Found a few things that very likely need changed - left comments about those inline. That should help slim things down a bit.

In general - I'm sympathetic to the cosmetic changes you've considered in this PR but I'm having trouble reviewing the PR in detail. I just don't know our "system info" stuff to tell at a glance what's "new" and what's a refactor of the existing code.

@igiguere would you object to our making this "just" a verbatim JAX-RS migration, and we could handle any cosmetic changes in a subsequent PR? (You could do this, if willing. Or if not, I'm happy to make the changes myself if you don't mind. I've waffled a bit on this already and don't want that to cause you additional work. Just give me a thumbs up or thumbs down and I can get started)

In practice this would mean:

* deleting the existing class NodeSystemInfoAPI

* Modifying NodeSystemInfoApi to use the "/node/system" path (at least, for now)

* Deleting `NodeSystemInfoApi.getSpecificNodeSystemInfo` (at least, for now)

* removing the `NodeSystemInfo` wrapper-object in `NodeSystemResponse` (again - just for now.  Of all your cosmetic improvements this is the one that I like the most! but it adds a ton to the diff and it'd be easier to review in a separate PR)

@gerlowskija:
I reverted changes to the response structure (nodeInfo wrapper), I removed the old V2 API NodeSystemInfoAPI and related unit tests. The new V2 NodeSystemInfoApi now uses the "/node/system" path. I deleted NodeSystemInfoApi.getSpecificNodeSystemInfo, along with it's implementation and unit test.

I hope it helps review.

I'm going through your other comments, to see if anything else still applies.

Isabelle Giguere added 2 commits March 2, 2026 13:11
adjust changelog and run tidy
run gradlew check, fix logging "issue".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:api client:solrj documentation Improvements or additions to documentation tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants