SOLR-17136: Replace most uses of GenericSolrRequest in Solr code #3955

igiguere · 2025-12-16T23:42:49Z

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

Description

Replacing uses of GenericSolrRequest to call "/admin/info/system" and "admin/metrics" by classes SystemInfoRequest, and MetricsRequest.

Solution

Adding class SystemInfoRequest. SystemInfoRequest can toggle between the V1 and V2 APIs. The V2 API was already available at /api/node/system.

Adding API model class org.apache.solr.api.model.SystemInfoResponse to represent the system info response, and SorlJ response class org.apache.solr.client.solrj.response.SystemInfoResponse to map the NamedList response to the API model.

Adding class MetricsRequest. This one returns a generic SolrResponse, because there are so many variants of a metrics response.

Replacing hard-coded strings "/admin/info/system" and "admin/metrics" by the corresponding constants.

Tests

Unit tests pass. Existing unit tests already serve to test these modifications.

Functional tests using the CLI tool, where most of the functional changes occurred.

Multiple changes in unit tests, to use the new classes, or existing constants.

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

SystemInfoResponse

SystemInfoRequest/Response in CLI and package manager

Adjust the API model SystemInfoResponse and tie it into the SolrResponseBase. Remove commented code.

epugh · 2025-12-17T00:32:31Z

I approved the workflows to run. Ping us when you are ready for review!

add MetricsRequest, more replacements of GenericSolrRequest, use the constants for "/admin/info/system" and "/admin/metrics"

…m:igiguere/solr.git into SOLR-17136-replace-GenericSolrRequest

solr/solrj/src/java/org/apache/solr/client/solrj/request/MetricsRequest.java

solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java

igiguere · 2025-12-17T23:50:38Z

Hemm. I wanted to run gradlew tidy with my corrections for the comments, but I get this error. My branch is up to date with main.
Running with the --stacktrace and --debug options shows a NoClassDefFoundError about com/diffplug/gradle/spotless/SpotlessPluginRedirect
I cleared gradle caches, but that did not help.

$ ./gradlew tidy

FAILURE: Build failed with an exception.

Where:
Settings file '/opt/github/apache-solr/settings.gradle' line: 28
What went wrong:
Error resolving plugin [id: 'com.gradle.develocity', version: '3.18.2']

A problem occurred configuring project ':build-infra'.
A problem occurred evaluating project ':build-infra'.
> A problem occurred evaluating script.
> com/diffplug/gradle/spotless/SpotlessPluginRedirect

UPDATE:
I ran a gradle task from inside Eclipse, and now, te command line gradlew seems to be magically fixed. Weird.

epugh

This is super exciting! I didn't quite realize how much duplicative boilerplate using GSR brought.

I think this is mostly in the right direction. A couple of comments and questions. I don't see explicit tests for these new classes, though I guess they are literally excercised everywhere and are just wrapper classes.

solr/core/src/java/org/apache/solr/cli/StatusTool.java

epugh · 2025-12-18T12:06:03Z

solr/core/src/java/org/apache/solr/cli/StatusTool.java

-      String zkHost = (String) info.get("zkHost");
-      status.put("cloud", getCloudStatus(solrClient, zkHost));
+    if ("solrcloud".equals(sysResponse.getMode())) {
+      status.put("cloud", getCloudStatus(solrClient, sysResponse.getZkHost()));


LIkewise in a future pr, would be nice to figure out is it cloud or solrcloud and use a single term everywhere ;-)

epugh · 2025-12-18T12:06:19Z

solr/core/src/java/org/apache/solr/cli/HealthcheckTool.java

Lovely change!

epugh · 2025-12-18T12:07:07Z

solr/core/src/java/org/apache/solr/cli/CreateTool.java

-    coreRootDirectory = (String) systemInfo.get("core_root");
+    SystemInfoResponse sysResponse = (new SystemInfoRequest()).process(solrClient);
+    // usually same as solr home, but not always
+    String coreRootDirectory =


can core_root ever be null? do we know we need this check?

No idea. Just being cautious. Although, the original code did not check for null, so I guess the new code shouldn't need to.

epugh · 2025-12-18T12:08:34Z

solr/core/src/java/org/apache/solr/cli/CLIUtils.java

-
-      // convert raw JSON into user-friendly output
-      Map<String, Object> status = StatusTool.reportStatus(systemInfo, solrClient);
+      Map<String, Object> status = StatusTool.reportStatus(solrClient);


maybe for future, but is it odd a CLIUtils would call StatusTool for a method reportStatus? Should it be moved to CLIUtils instead and the StatusTool should call CLIUtils.reportStatus?

There are 17 calls to CLIUtils.getZkHost(CommandLine), where we find this call to StatusTool.reportStatus(SolrClient).
That's half the *Tool found in org.apache.solr.cli. That means CLIUtils serves as a "bridge" between tools (StatusTool and the rest of them).
Personally, I think it makes sense that the StatusTool would be the one reporting the status, rather than have it depend on the CLIUtils to essentially do the status reporting job (i.e.: CLIUtils.reportStatus).
Otherwise, if we don't want CLIUtils to call StatusTool, then, we have to add a call to StatusTool.reportStatus(SolrClient) in each of the 17 locations, and for each, find the ZK host currently provided by CLIUtils.getZkHost(CommandLine).

In a nutchell : I would not change this.

makes sense, thanks for digging into this.

epugh · 2025-12-18T12:09:34Z

solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java

+    // This method is only called from PackageTool ("add-repo", or "add-key"), where the Solr URL is
+    // normalized to remove the /solr path part
+    // So might as well ping the V2 API "/node/system" instead.
+    // Otherwise, this SystemInfoRequest ctr would need to set the full /solr/admin/info/system path


"ctr"??? maybe spell it out.

;) it would have wrapped the line, but ok. I'll spell it out.

epugh · 2025-12-18T12:11:20Z

solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java

+    // normalized to remove the /solr path part
+    // So might as well ping the V2 API "/node/system" instead.
+    // Otherwise, this SystemInfoRequest ctr would need to set the full /solr/admin/info/system path
+    SystemInfoResponse sysResponse = new SystemInfoRequest("/node/system").process(solrClient);


humm.... In general we should just use V2 api's! Over time in Solr 10 x we will migrate internal tooling to using V2 apis, which will open the door to deprecate and remove V1 equivalents. If I am understanding here, you are choosing to use V2 in this one use case. Could we just use V2 everywhere....

SystemInfoRequest could set the V2 path by default.

I was going to set the V2 path /node/system as default in SystemInfoRequest constructors, but, that would require some refactoring throughout the CLI tools, at least. The SolrClient used for the system info can be reused for other request that don't all support V2, so, having SystemInfoRequest default to V2 would mean managing the switch from V1 to V2 URLs.
Let's takle that in some other ticket. I'm leaving a "TODO" for now.

makes sense.

epugh · 2025-12-18T12:13:51Z

solr/core/src/test/org/apache/solr/handler/admin/AdminHandlersProxyTest.java

-                  params);
-          SimpleSolrResponse rsp = null;
+          SystemInfoRequest req = new SystemInfoRequest(params);
+          SystemInfoResponse rsp = null;


check your ide, I think in geneal we don't do the = null pattern when declaring an empty variable as it is already null.

It was like that already, but for SimpleSolrResponse. I'll remove the = null.

Update: It was initialized to avoid compile error at line 91, after the try-catch, when rsp is used: "variable rsp might not have been initialized".
So I'll move the following asserts into the try-catch.

epugh · 2025-12-18T12:16:25Z

solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java

+
+  private static final long serialVersionUID = 1L;
+
+  private org.apache.solr.client.api.model.SystemInfoResponse fullResponse;


the import path being full here?

Import the API model SystemInfoResponse into the SolrJ client SystemInfoResponse. Maybe I should rename one of these classes, to avoid confusion ?

@gerlowskija any suggestions on what we should do here? Having the same name in two packages seems not good. SystemInfo?

In org.apache.solr.client.api.*:
Most API model classes that represent responses have a name in *Reponse. There's also some *RequestBody. There seems to be some mapping between the class name and the exposed resource path (but not 100%)
So I think the system info response should have a name that ends in *Response. And I like the idea of keeping names aligned ("/whatever" resource = WhateverAPI returns WhateverResponse, and Whatever implements WhateverAPI)
The "home-grown" V2 NodeSystemInfoAPI exposes resource path "/node/system". Presumably, the JAX-RS V2 would still have a class name similar to it's resource path (i.e.: NodeSystemAPI ?) ... Which would make the API model response class NodeSystemResponse ?
That's my 2 cents. @epugh, @gerlowskija

I think I can live with NodeSystemAPI and NodeSystemResponse. One thing I hate is having a PR sit forever ont he vine because we can't come up whti a name. We can always rename these thingsin future versions!

I just saw that comment. Renamed.

Done. Unless checks fail.

epugh · 2025-12-18T12:17:25Z

solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java

+  }
+
+  // *************************
+  // Shortcuts


is this a pattern in other similar classes? Mostly my own curiosity!

I've seen some similar "separator" comments in other classes, can't remember which right now. I can remove it.
OK, no, you meant the methods themselves. Ref. CoreAdminResponse providing similar methods : getStartTime, getUpTime. Also DocumentAnalysissResponse. If the response is complex enough, some parsing should be provided so the rest of the code doesn't need to know the inner structure of the response.

epugh · 2025-12-18T12:22:09Z

@gerlowskija can you review from the v2 api perspective? If @igiguere is intered, I'd love to take what she's learned on making V2 api for metrics and system request handler and look at osme of our other V2 api gaps!

igiguere · 2025-12-18T15:26:15Z

@gerlowskija can you review from the v2 api perspective? If @igiguere is intered, I'd love to take what she's learned on making V2 api for metrics and system request handler and look at osme of our other V2 api gaps!

FYI : System Info already had a V2 implementation (/node/system), I didn't touch it. It just calls the SystemInfoHandler, same as V1. I didn't see a V2 resource for metrics. I suppose it could also call the same handler as V1.
And I wouldn't say I "learned" a lot about V2... I just added a toggle between paths in getApiVersion() ! It saves writing a V2SystemInfoRequest.

epugh · 2025-12-18T17:26:25Z

FYI: https://docs.google.com/spreadsheets/d/1HAoBBFPpSiT8mJmgNZKkZAPwfCfPvlc08m5jz3fQBpA/edit?gid=1549066254#gid=1549066254

epugh · 2025-12-20T12:29:25Z

I just approved the workflows... Assuming the tests all pass, I'l leave it open till Monday to give folks a chance to review and then merge. This looks great... What's next?

igiguere · 2025-12-20T17:44:46Z

I just approved the workflows... Assuming the tests all pass, I'l leave it open till Monday to give folks a chance to review and then merge. This looks great... What's next?

I'm running one more tidy and check locally. Seems there's some un-tidy files, even I haven't touched them. There's one failing test in the last run that I can't reproduce locally.
There's a warning about some osgi files, I don't know where that comes from. I didn't add any dependencies.
WARNING: there were unreferenced files under license folder:

/home/runner/work/solr/solr/solr/licenses/osgi.annotation-LICENSE-ASL.txt
/home/runner/work/solr/solr/solr/licenses/osgi.annotation-NOTICE.txt

What's next? For me, in the next few days: get over my cold that's dragging on, find a plumber to fix the clogged drain, Christmas... After that, more job hunting, and possibly tackle some of the missing V2 APIs.

epugh · 2025-12-20T18:36:51Z

Pre commit failed. The crave tests have been failing

igiguere · 2025-12-20T19:35:51Z

Sigh. I ran gradlew check locally: I don't reproduce the same failures as the Crave tests, but I do get a few failures. When I run each failed test class one by one (gradlew test --tests=TestClass), then none of the tests fail. I can only suppose there's some conflict, race condition, when tests are running concurrently.
I also ran gradlew tidy locally one more time, but it produced no file changes.
I have no idea what's wrong here.

epugh · 2025-12-20T20:55:25Z

update from main now.. there was a lint and extra dependency that broke main a bit...
I will check your branch out and run the tests myself too!

epugh · 2025-12-20T20:56:46Z

actually, I just pushed up the updates from main and am running tests

epugh · 2025-12-20T21:17:19Z

TEsts passed! I will leave this open for mOnday, and then will merge. Please tag me on your next PR, enjoyed working with you on this.

Isabelle Giguere and others added 5 commits December 12, 2025 11:17

SOLR-17136 initial commit: classes SystemInfoRequest and

80e2582

SystemInfoResponse

SOLR-17136: integrate SystemInfoRequest/Response

226448b

SystemInfoRequest/Response in CLI and package manager

Merge branch 'apache:main' into SOLR-17136-replace-GenericSolrRequest

f79c5ae

SOLR-17136: replace GenericSolrRequest

2470367

Adjust the API model SystemInfoResponse and tie it into the SolrResponseBase. Remove commented code.

SOLR-17136: add changelog file

eec0643

github-actions bot added client:solrj tests cat:cli cat:packagemanager labels Dec 16, 2025

Isabelle Giguere and others added 3 commits December 17, 2025 16:45

SOLR-17136: more replacements of GenericSolrRequest

e219435

add MetricsRequest, more replacements of GenericSolrRequest, use the constants for "/admin/info/system" and "/admin/metrics"

Merge branch 'apache:main' into SOLR-17136-replace-GenericSolrRequest

74df237

Merge branch 'SOLR-17136-replace-GenericSolrRequest' of [email protected]…

0c11fc1

…m:igiguere/solr.git into SOLR-17136-replace-GenericSolrRequest

github-actions bot added test-framework module:jwt-auth module:opentelemetry cat:search cat:cloud cat:api cat:security labels Dec 17, 2025

igiguere marked this pull request as ready for review December 17, 2025 21:56

mlbiscoc reviewed Dec 17, 2025

View reviewed changes

solr/solrj/src/java/org/apache/solr/client/solrj/request/MetricsRequest.java Outdated Show resolved Hide resolved

solr/solrj/src/java/org/apache/solr/client/solrj/response/SystemInfoResponse.java Outdated Show resolved Hide resolved

Merge branch 'apache:main' into SOLR-17136-replace-GenericSolrRequest

712ce54

Isabelle Giguere added 2 commits December 17, 2025 19:17

SOLR-17136: address code review comments. Remove permissive constructors

955fc46

SOLR-17136: Adjust SystemInfoResponse with new "gpu" field

f09182d

epugh reviewed Dec 18, 2025

View reviewed changes

epugh requested a review from gerlowskija December 18, 2025 12:20

Isabelle Giguere added 2 commits December 19, 2025 15:50

SOLR-17136: address review comments

bd60a18

SOLR-17136: rename API model SystemInfoResponse to NodeSystemResponse

d335770

Merge remote-tracking branch 'upstream/main' into pr/3955

5b106e7


		private static final long serialVersionUID = 1L;

		private org.apache.solr.client.api.model.SystemInfoResponse fullResponse;

SOLR-17136: Replace most uses of GenericSolrRequest in Solr code #3955

Are you sure you want to change the base?

SOLR-17136: Replace most uses of GenericSolrRequest in Solr code #3955

Uh oh!

Conversation

igiguere commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

epugh commented Dec 17, 2025

Uh oh!

Uh oh!

Uh oh!

igiguere commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igiguere Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igiguere Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igiguere Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epugh commented Dec 18, 2025

Uh oh!

igiguere commented Dec 18, 2025

Uh oh!

epugh commented Dec 18, 2025

igiguere commented Dec 16, 2025 •

edited

Loading

igiguere commented Dec 17, 2025 •

edited

Loading

igiguere Dec 18, 2025 •

edited

Loading

igiguere Dec 18, 2025 •

edited

Loading

igiguere Dec 18, 2025 •

edited

Loading