Refactor CLI StreamTool SolrClientCache usage#4513
Conversation
dsmiley
commented
Jun 9, 2026
dsmiley
commented
- don't call SolrClientCache.setBasicAuthCredentials, to be deprecated soon (primary motivation). Instead set this on the client passed into SCC.
- insist on a StreamContext, even if "remote mode". Code was a bit of a mess, doing redundant things or not doing what it "should" do
* don't call SolrClientCache.setBasicAuthCredentials * insist on a StreamContext, even if "remote mode"
|
Disclaimer: I haven't tested or even used it in a realistic way. |
| * @return the solrUrl in the format that Solr expects to see internally. | ||
| * @return a URL without any path, e.g. {@code http://localhost:8983} | ||
| */ | ||
| public static String normalizeSolrUrl(String solrUrl) { |
There was a problem hiding this comment.
IMO these "normalize" methods are non-obvious since the URL normalization I'm used to seeing within Solr produce Solr URLs ending with "/solr". The javadoc here refers to what I'm used to as "legacy". Wow... am I a dinosaur or has v2 taken over? SolrJ didn't get the memo, I can tell you that.
There was a problem hiding this comment.
I think a little bit dinosaur... But not a lot dinosaur... We've been trying to simpliy users lives so they don't have to think about /solr or /v2 or /api all the time, unless is critical. So, we normalize to the root version, and then whatever tool can append what it needs...
There was a problem hiding this comment.
IMO URL utility methods should be centralized to URLUtil. And naming reconsidered. If this method produces URLs without any path, then this is an opportunity to very simply state that in the method name rather than ambiguous "normalization", (it once wasn't ambiguous but in our transition period, is ambiguous).
|
Is #4510 potentially overlapping with this work? |
| } | ||
| HttpJettySolrClient client = jettyClientBuilder.build(); | ||
|
|
||
| // subclass so we can ensure our client is closed when the cache is closed |
There was a problem hiding this comment.
This is some magic! If this isn't there, do we see errors?
| } | ||
| return new PushBackStream(solrStream); | ||
| return new PushBackStream( | ||
| new SolrStream(solrUrl + "/solr/" + collection, params("qt", "/stream", "expr", expr))); |
There was a problem hiding this comment.
i feel like this should be "/solr/" + collection + "/stream", params("expr",expr)... but that is a bigger change.
Signed-off-by: Eric Pugh <epugh@opensourceconnections.com>
2 participants
