SOLR-16458: Node system info V2 api jersey resource#4078
SOLR-16458: Node system info V2 api jersey resource#4078igiguere wants to merge 30 commits intoapache:mainfrom
Conversation
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
Clean-up unused imports
forgot those in the previous commit
Fix log calls. Add unit tests for HTTP call.
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.
|
First off, thanks again for sharing this PR and tackling another v2 API!
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 |
Thanks for the feedback, @gerlowskija 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 |
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.
Merge remote-tracking branch 'origin-solr/main' into NodeSystemInfoApi-JerseyResource
Merge remote-tracking branch 'origin-solr/main' into NodeSystemInfoApi-JerseyResource
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
using the PR rather that the mixed-up Jira ticket
| * @deprecated Use the JAX-RS API: NodeSystemInfo (/node/info/system), implementing NodeSystemApi, | ||
| * and returning NodeSystemResponse. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Yep, that was my first question, in the description. I can delete the class, no problem.
| /** | ||
| * @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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Looking forward to enhancing the Solr CLI!
epugh
left a comment
There was a problem hiding this comment.
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?
|
Also, I think this may have some overlap? #4171 |
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
left a comment
There was a problem hiding this comment.
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
NodeSystemInfowrapper-object inNodeSystemResponse(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 | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
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( |
There was a problem hiding this 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.
There was a problem hiding this comment.
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"; |
There was a problem hiding this 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.
There was a problem hiding this comment.
Removed in the latest commit.
| * @deprecated Use the JAX-RS API: NodeSystemInfo (/node/info/system), implementing NodeSystemApi, | ||
| * and returning NodeSystemResponse. | ||
| */ | ||
| @Deprecated |
| @@ -0,0 +1,74 @@ | |||
| /* | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoV2Response.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,103 @@ | |||
| /* | |||
There was a problem hiding this 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?
There was a problem hiding this comment.
Class deleted in the latest commit
|
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. |
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.
@gerlowskija: I hope it helps review. I'm going through your other comments, to see if anything else still applies. |
adjust changelog and run tidy
run gradlew check, fix logging "issue".
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:
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"
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:
mainbranch../gradlew check.