SOLR-16458: Convert /api/node/system to JAX-RS

[igiguere]

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

[gerlowskija]

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]

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?

[epugh]

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

[igiguere]

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]

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.

[gerlowskija]

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)

[epugh]

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]

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.

[igiguere]

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.

[gerlowskija]

Hey @igiguere - I left one last round of review. Most of the changes were minor/tedious, so I've taken the liberty of making them myself and pushing to your branch. As such the comments should all be "addressed" and not hugely worth reading, but I still figured I'd leave them as a way to flag what I changed and give a bit of background on the rationale.

Gonna aim to commit this in the next day or two. Thanks for all your patience and effort on this; exciting to get it merged at long last!

[igiguere]

Hey @igiguere - I left one last round of review. Most of the changes were minor/tedious, so I've taken the liberty of making them myself and pushing to your branch. As such the comments should all be "addressed" and not hugely worth reading, but I still figured I'd leave them as a way to flag what I changed and give a bit of background on the rationale.

Gonna aim to commit this in the next day or two. Thanks for all your patience and effort on this; exciting to get it merged at long last!

Thanks for the fixes, @gerlowskija !
I have read your comments, and resolved the conversations, since it was all fixed. Except for the change in SystemInfoHandler, lines 60+ : that was for back-compatibility of the V1 response, so I think the core info should go back in the V1 System info response. Unless the new CoreInfoHandler added by @epugh is already available in a released version? But, even if that's the case, why unplug everything at once? And note that there's no V2 for CoreInfoHandler.
Anyways. You're the committers ;)

[gerlowskija]

Excellent - appreciate the double check on those comments.

Except for the change in SystemInfoHandler, lines 60+ : that was for back-compatibility of the V1 response, so I think the core info should go back in the V1 System info response.

Can you elaborate about the backcompat break you're seeing there?

It's very possible I'm missing something, but my understanding is that even though the code was there to do so, /admin/info/system never returned a core field in its response because req.getCore() is always null when the API is accessed at that path.

The conditional needed to exist at one point because SystemInfoHandler used to serve /solr/<coreName>/admin/info as well. But since that's no longer the case then the conditional in SystemInfoHandler is essentially dead code and can be removed without actually changing the response in any way.

What am I missing?

[gerlowskija]

Oops - forgot to tag @igiguere .

Also...

note that there's no V2 for CoreInfoHandler.

Good call out - I'll fix that in a quick PR once this gets merged! I think we could add that v2 API in this PR, but I'd like to get this one merged since it's been outstanding for so long.

[igiguere]

The conditional needed to exist at one point because SystemInfoHandler used to serve /solr/<coreName>/admin/info as well. But since that's no longer the case then the conditional in SystemInfoHandler is essentially dead code and can be removed without actually changing the response in any way.

@gerlowskija : Quick test on a Solr 9.10. /solr/<coreName>/admin/info returns just the error message "Looking for Solr?...". So you are right, it is dead code.

[epugh]

Awesome work team!

[igiguere]

Yeah! Merged !
Thanks for that test fix, @gerlowskija
@epugh , @gerlowskija : I have created this ticket for the follow-up https://issues.apache.org/jira/browse/SOLR-18161