SOLR-17625 NamedList.findRecursive-> _get #3355

wildtusker · 2025-05-16T15:05:24Z

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

dsmiley

I suspect you didn't review your work / the results.

You had List.of(new String[]{"foo", "bar"}) in all these places instead of List.of("foo", "bar"). Using IntelliJ "structural replace" feature, I easily fixed all of them at once and pushed to your PR.

There are newly declared variables everywhere. That should be completely unnecessary. You can fix them by doing doing "inline" refactoring in IntelliJ when clicking each needless variable (use a hot key to make it go quicker).

In places a String is casted, _getStr should be used.

In some places, I expect the default value (last arg) to be useful, which findRecursive didn't support.

All this makes this change look way worse but instead we should find essentially the same amount of code as before, mostly.

dsmiley · 2025-05-17T18:30:56Z

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

@@ -280,7 +281,8 @@ public int assertSolrNotRunning(String url, String credentials) throws Exception
        System.nanoTime() + TimeUnit.NANOSECONDS.convert(timeoutMs, TimeUnit.MILLISECONDS);
    try (SolrClient solrClient = CLIUtils.getSolrClient(url, credentials)) {
      NamedList<Object> response = solrClient.request(new HealthCheckRequest());
-      Integer statusCode = (Integer) response.findRecursive("responseHeader", "status");
+      Object value = response._get(List.of(new String[] {"responseHeader", "status"}), null);
+      Integer statusCode = (Integer) value;


Don't declare new variables merely to cast.

dsmiley · 2025-05-17T18:47:01Z

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

-              String totalMemory = (String) systemInfo.findRecursive("jvm", "memory", "total");
+              Object value2 = systemInfo._get(List.of("jvm", "jmx", "upTimeMS"), null);
+              uptime = SolrCLI.uptime((Long) value2);
+              Object value1 = systemInfo._get(List.of("jvm", "memory", "used"), null);


use _getStr, same with below

dsmiley · 2025-05-17T18:49:30Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java

@@ -862,14 +862,18 @@ public static void checkDiskSpace(
                SolrRequest.METHOD.GET, "/admin/metrics", SolrRequest.SolrRequestType.ADMIN, params)
            .process(cloudManager.getSolrClient());

-    Number size = (Number) rsp.getResponse().findRecursive("metrics", indexSizeMetricName);
+    NamedList<Object> entries1 = rsp.getResponse();


again, no need to declare another variable

still an issue

This reverts commit 109d8ff.

wildtusker · 2025-05-23T08:45:01Z

List.of(new String[]{"foo", "bar"})
Made changes using Method Over loading .Got rid of List.of(new String[]{"foo", "bar"}) .Even the variables which were getting created
Newly declared variables have been removed

4_getStr has not been used as of now.
Default value will be on case to case basis or where required.

dsmiley

I did a partial review. I encourage you to review it fully (I do it for my own work).

I get the feeling that _get ought not to have a default parameter (at the end) since it's so rarely useful, judging from this PR anyway. If that makes sense; it'd be a separate PR later.

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

dsmiley · 2025-05-23T16:56:18Z

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

+    String usedMemory = (String) info._get(List.of("jvm", "memory", "used"), null);
+    String totalMemory = (String) info._get(List.of("jvm", "memory", "total"), null);


Will make the change. _get is replaced with _getStr.

dsmiley · 2025-05-23T16:57:35Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java

@@ -862,14 +862,18 @@ public static void checkDiskSpace(
                SolrRequest.METHOD.GET, "/admin/metrics", SolrRequest.SolrRequestType.ADMIN, params)
            .process(cloudManager.getSolrClient());

-    Number size = (Number) rsp.getResponse().findRecursive("metrics", indexSizeMetricName);
+    NamedList<Object> entries1 = rsp.getResponse();


still an issue

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type Updated StatusTool.java SplitShardCmd.java doesnt require update

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

dsmiley · 2025-05-27T22:29:47Z

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

-              String totalMemory = (String) systemInfo.findRecursive("jvm", "memory", "total");
+              uptime =
+                  SolrCLI.uptime((Long) systemInfo._get(List.of("jvm", "jmx", "upTimeMS"), null));
+              String usedMemory = (String) systemInfo._get(List.of("jvm", "memory", "used"), null);


I assume you didn't finish reviewing this PR since this shows some _getStr opportunities.

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

dsmiley

Please self review more next time so that you believe it to be ready. There were dubious entries1 that we discussed previously.

Otherwise looks good!

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

solr/core/src/test/org/apache/solr/search/facet/SpatialHeatmapFacetsTest.java

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

dsmiley · 2025-05-29T17:07:30Z

solr/core/src/test/org/apache/solr/search/facet/SpatialHeatmapFacetsTest.java

@@ -438,8 +436,7 @@ public void testJsonFacets() throws Exception {
                      + " } "
                      + "}"));
      {
-        NamedList<Object> entries1 = response.getResponse();
-        final NamedList<?> q1Res = (NamedList<?>) entries1._get(List.of("facets", "q1"), null);
+          final NamedList<?> q1Res = (NamedList<?>) response.getResponse()._get(List.of("facets", "q1"), null);


remember "gw tidy", I see here and elsewhere. I think you can configure IntelliJ better so that basic indentation like this would not be incorrect so often. I have an idea that I'll raise elsewhere that should reduce this annoyance.

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

Replaced NamedList.findRecursive usages with _get, which can do Map traversal, and thus makes it easier to transition intermediate NamedLists to Maps. (cherry picked from commit 51841d7)

SOLR-17625 NamedList.findRecursive-> _get

81eed8a

github-actions bot added test-framework client:solrj tests cat:search cat:cloud cat:cli cat:api cat:packagemanager labels May 16, 2025

Replace List.of(array) direct arguments

109d8ff

dsmiley requested changes May 17, 2025

View reviewed changes

Revert "Replace List.of(array) direct arguments"

edf1444

This reverts commit 109d8ff.

dsmiley reviewed May 23, 2025

View reviewed changes

wildtusker added 2 commits May 24, 2025 23:18

Revert "SOLR-17625 NamedList.findRecursive-> _get"

7469971

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type Updated StatusTool.java SplitShardCmd.java doesnt require update

Revert "SOLR-17625 NamedList.findRecursive-> _get"

2de2b7f

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type Updated StatusTool.java SplitShardCmd.java doesnt require update

wildtusker force-pushed the SOLR-17625 branch from 556fc38 to 2de2b7f Compare May 27, 2025 14:03

Revert "SOLR-17625 NamedList.findRecursive-> _get"

6ca7b33

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

dsmiley reviewed May 27, 2025

View reviewed changes

Revert "SOLR-17625 NamedList.findRecursive-> _get"

811f31a

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

dsmiley reviewed May 29, 2025

View reviewed changes

Revert "SOLR-17625 NamedList.findRecursive-> _get"

5d2fc5d

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

dsmiley reviewed May 29, 2025

View reviewed changes

wildtusker and others added 5 commits May 31, 2025 23:29

Revert "SOLR-17625 NamedList.findRecursive-> _get"

8d7bd70

This reverts commit 81eed8a. Changes made with help inline method feature.The methods were over loaded with 1 , 2 ,3 and 4 arguments with String type update

CHANGES.txt

3139ee0

revert

1d7d2a0

Merge branch 'main' into fork/wildtusker/SOLR-17625

752db12

CHANGES.txt

32c245b

dsmiley merged commit 51841d7 into apache:main Jun 2, 2025
1 check passed

		String usedMemory = (String) info._get(List.of("jvm", "memory", "used"), null);
		String totalMemory = (String) info._get(List.of("jvm", "memory", "total"), null);

SOLR-17625 NamedList.findRecursive-> _get #3355

SOLR-17625 NamedList.findRecursive-> _get #3355

Uh oh!

Conversation

wildtusker commented May 16, 2025

Uh oh!

dsmiley left a comment

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!

wildtusker commented May 23, 2025

Uh oh!

dsmiley 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!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!