SOLR-17625 NamedList.findRecursive-> _get #3355
Conversation
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
Don't declare new variables merely to cast.
| 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); |
There was a problem hiding this comment.
use _getStr, same with below
| @@ -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(); | |||
There was a problem hiding this comment.
again, no need to declare another variable
This reverts commit 109d8ff.
4_getStr has not been used as of now. |
There was a problem hiding this comment.
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.
| String usedMemory = (String) info._get(List.of("jvm", "memory", "used"), null); | ||
| String totalMemory = (String) info._get(List.of("jvm", "memory", "total"), null); |
There was a problem hiding this comment.
Will make the change. _get is replaced with _getStr.
| @@ -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(); | |||
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 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
| 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); |
There was a problem hiding this comment.
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
| @@ -655,7 +657,8 @@ public void testExprMetrics() throws Exception { | |||
| key2), | |||
| resp); | |||
| // response structure is like in the case of non-key params | |||
| val = resp.getValues().findRecursive("metrics", "solr.core.collection1"); | |||
| NamedList<Object> entries1 = resp.getValues(); | |||
| @@ -689,7 +692,8 @@ public void testExprMetrics() throws Exception { | |||
| MetricsHandler.EXPR_PARAM, | |||
| key3), | |||
| resp); | |||
| val = resp.getValues().findRecursive("metrics", "solr.core.collection1"); | |||
| NamedList<Object> entries = resp.getValues(); | |||
| @@ -148,16 +148,13 @@ public void testClassicFacets() throws Exception { // AKA SimpleFacets | |||
| + FIELD); | |||
| final QueryResponse response = query(params); | |||
| assertEquals(6, getHmObj(response).get("gridLevel")); // same test as above | |||
| NamedList<Object> entries1 = response.getResponse(); | |||
| .getResponse() | ||
| .findRecursive("facet_counts", "facet_heatmaps", "course", "gridLevel")); | ||
| 2, entries1._get(List.of("facet_counts", "facet_heatmaps", "course", "gridLevel"), null)); | ||
| NamedList<Object> entries = response.getResponse(); |
| @@ -441,11 +438,11 @@ public void testJsonFacets() throws Exception { | |||
| + " } " | |||
| + "}")); | |||
| { | |||
| final NamedList<?> q1Res = | |||
| (NamedList<?>) response.getResponse().findRecursive("facets", "q1"); | |||
| NamedList<Object> entries1 = response.getResponse(); | |||
There was a problem hiding this comment.
inline. In general, if we have a var name blahBlah1 (number at end), you should be highly suspicious that it's good as is. Inline or rename. I vote inline here.
| if (classicResp != null) { | ||
| return classicResp; | ||
| } | ||
| // JSON Facet | ||
| return (NamedList<?>) response.getResponse().findRecursive("facets", "f1"); | ||
| NamedList<Object> entries = response.getResponse(); |
| @@ -505,14 +502,15 @@ public void testJsonFacets() throws Exception { | |||
|
|
|||
| private NamedList<?> getHmObj(QueryResponse response) { | |||
| // classic faceting | |||
| NamedList<Object> entries1 = response.getResponse(); | |||
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
| @@ -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); | |||
There was a problem hiding this comment.
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.
https://issues.apache.org/jira/browse/SOLR-17625