dsmiley commented on code in PR #3156:
URL: https://github.com/apache/solr/pull/3156#discussion_r1939799714
##########
solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java:
##########
@@ -975,7 +975,7 @@ public Map<String, Object> execute(
CLUSTERSTATUS,
(req, rsp, h) -> {
new
ClusterStatus(h.coreContainer.getZkController().getZkStateReader(),
req.getParams())
- .getClusterStatus(rsp.getValues());
+ .getClusterStatus(rsp.getValues(),
req.getHttpSolrCall().getUserAgentSolrVersion());
Review Comment:
hmmm; I wonder if HttpSolrCall could be null. No; we don't use
CollectionsHandler in SolrCloud with an EmbeddedSolrServer. I *really* wish
Java had a null-safe navigation operator as used in Kotlin -- `?.`. I suppose
leave this be; not worth adding verbosity for an exotic / hypothetical case.
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java:
##########
@@ -487,7 +489,7 @@ private void clusterStatusWithCollection() throws
IOException, SolrServerExcepti
NamedList<Object> rsp = client.request(request);
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
- NamedList<?> collections = (NamedList<?>) cluster.get("collections");
+ Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
Review Comment:
very minor: consider use of `var` in any place where we cast so we don't
repeat the type. But again this is minor; feel free to just let this be and
consider this advice to future changes.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
Review Comment:
I appreciate your thoroughness. I should merge this as-is.
But...
In an ideal world, you wouldn't have had any need to do this -- no time
spent for you or an explicit test here to maintain in perpetuity. Solr is
lacking backwards-compatibility test infrastructure. A hello-world SolrCloud
would have caught this problem. Such a thing would nowadays almost certainly
use Docker (to run old Solr versions) and use `TestContainers` in Java to
arrange for that. We do this at work, albeit not for backwards-compatibility
but more for realistic / production like testing. The comment thread in
https://issues.apache.org/jira/browse/SOLR-14903 gets at this. I should create
a JIRA issue to spell out this need more. Really should be on our roadmap.
I'll do that too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
