SOLR-16800: Move towards using the root of Solr as the -solrUrl in the CLI. #1808
Conversation
| @@ -383,6 +383,11 @@ public static boolean exceptionIsAuthRelated(Exception exc) { | |||
| } | |||
|
|
|||
| public static SolrClient getSolrClient(String solrUrl) { | |||
| // today we require all urls to end in /solr, however in the future we will need to support the | |||
| // /api url end point instead. | |||
| if (!solrUrl.endsWith(("/solr"))) { | |||
There was a problem hiding this comment.
[0] Not sure whether this is intended or not, but if the provided URL was http://localhost:8983/solr/ (not the ending '/') we'd still append/solr. I wonder if there isn't some Java utility for adding path segments to a URL that can be a little smarter about some of the edge cases (handling adjacent forward slashes, etc.)
There was a problem hiding this comment.
So this kind of made me sad... basically I was trying to write both code to handle legacy /solr type urls, but then, since everything TODAY requries a /solr url, then to inject it here. Have we figured out how in the future to hit a V2 api that has an /api end point instead via a SolrClient? My hope is once that is solved then we remove this logic.
Having said that, if there is better code to use here, would love some of that! I hope that before 10x gets released, this code will be eliminated, so I kind of just put this in "for now".... I don't expect to backport this to 9x.
There was a problem hiding this comment.
Ugh... One test has "http://127.0.0.1:51902/uf_" as the URL.....
There was a problem hiding this comment.
Oh wait... why do tests now have solr urls that end in /solr??? Now I wonder if they also stand up Solr servers that don't use /api for the v2 api endpoint?
There was a problem hiding this comment.
okay, I've pulled in main that has the now hardcoded hostContext...
| if (solrUrl.indexOf("/solr") > -1) { // | ||
| solrUrl = solrUrl.substring(0, solrUrl.indexOf("/solr")); | ||
| CLIO.out( | ||
| "The solrUrl parameter only needs to only point to the root of Solr (" |
There was a problem hiding this comment.
[-0] I think this message would read more clearly if you dropped the second "only".
[0] Maybe add some indication in this message that Solr is able to auto-correct the URL for folks, so they don't see the warning and think the command has failed. e.g.
'solrUrl' needn't include Solr's context-root (e.g. "/solr"); correcting from [old] to [new]
There was a problem hiding this comment.
Love it... pushing up fix.
|
This PR is held up on getting SOLR-16911, PR #1810 committed first.... |
|
@chatman I've got some optimizations to the Package tool if you want to look. @gerlowskija I'd love another looksee now that hostContext has been sorted.... |
epugh
dismissed
gerlowskija’s stale review
while clicking dismiss, it's because I responded to everything!
https://issues.apache.org/jira/browse/SOLR-16800
Description
In the future we'll have V2 API urls that are under /api, so having urls be /solr is a bit awkward. Allow -solrUrl http://localhost:8983 to work, and warn on http://localhost:8983/solr.
Some other cleanups as well.
Solution
Use the SolrCLI.getSolrClient method through out the client Tool's.
Tests
bats tests
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.