SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 #3156
Conversation
|
Meant to put into draft. A few more things I need to add. |
| NamedList<?> collections = (NamedList<?>) cluster.get("collections"); | ||
| Map<?, ?> collections = (Map<?, ?>) cluster.get("collections"); |
There was a problem hiding this 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.
| @@ -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()); | |||
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Understood. Would you prefer I just remove these backwards compatibility tests I wrote?
https://issues.apache.org/jira/browse/SOLR-17582
Description
Check for SolrJ version from a cluster status request user agent header to stream the response for any SolrJ clients >= 9.9 allowing for backwards compatibility.
Solution
Removed old Javabin check that didn't allow streaming for SolrJ and use the requests Solr version as the check instead for >= 9.9 to either stream or collect whole response in a
SimpleOrderedMapTests
Created
testClusterStateProviderBackwardCompatabilityto test backwards compatabilityChecklist
Please review the following and check all that apply:
mainbranch../gradlew check.