You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "mlbiscoc (via GitHub)" <gi...@apache.org> on 2025/02/03 16:26:10 UTC
[PR] SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 [solr]
mlbiscoc opened a new pull request, #3156:
URL: https://github.com/apache/solr/pull/3156
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 `SimpleOrderedMap`
# Tests
Created `testClusterStateProviderBackwardCompatability` to test backwards compatability
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) 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](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on PR #3156:
URL: https://github.com/apache/solr/pull/3156#issuecomment-2631588200
Meant to put into draft. A few more things I need to add.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #3156:
URL: https://github.com/apache/solr/pull/3156#discussion_r1941385619
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
Review Comment:
Understood. Would you prefer I just remove these backwards compatibility tests I wrote?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3156:
URL: https://github.com/apache/solr/pull/3156#discussion_r1941999333
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
Review Comment:
They have value; they should stay. If we finally get the backwards-compatibility tests, we could remove them then.
I filed https://issues.apache.org/jira/browse/SOLR-17653
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #3156:
URL: https://github.com/apache/solr/pull/3156
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org