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>
|
@dsmiley make sure after my chagnes we still are doing what you watned... I had to move some things around to make it work. Took opportunity to imrpvoe where validation of inputs takes place. I marked it as no-changelog as this isnt' user facing. |
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Just one little suggested change but looks good to me. Have you tried this code in any way? I have not.
|
i have tried it... so if thet ests all pass, I'll merge. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the bin/solr stream CLI implementation to stop using SolrClientCache.setBasicAuthCredentials (noted as soon-to-be deprecated) by instead wiring auth onto the HTTP client provided to SolrClientCache, and to consistently construct/use a StreamContext for both local and remote execution paths.
Changes:
- Reworked
StreamToolto always create aStreamContext(with aDefaultStreamFactory) and to pass a client-backedSolrClientCachethrough that context. - Removed direct credential setting on
SolrStreamin remote mode; credentials are now applied via the shared HTTP client/cached clients. - Updated
CLIUtils.normalizeSolrUrlJavadocs to clarify the return shape (bare base URL without path).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
solr/core/src/java/org/apache/solr/cli/StreamTool.java |
Refactors StreamContext/SolrClientCache lifecycle, credential handling, and local vs remote stream construction. |
solr/core/src/java/org/apache/solr/cli/CLIUtils.java |
Javadoc clarification for normalized Solr URL return format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (expr.toLowerCase(Locale.ROOT).contains("stdin(")) { | ||
| throw new IllegalStateException( | ||
| "The stdin() expression is only usable with --worker local set up."); | ||
| } |
| String credentials = cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION); | ||
| if (credentials != null) { | ||
| String[] userPass = credentials.split(":"); | ||
| jettyClientBuilder.withBasicAuthCredentials(userPass[0], userPass[1]); | ||
| } |
| @Override | ||
| public synchronized void close() { | ||
| super.close(); | ||
| client.close(); | ||
| } |
| var solrConnection = CLIUtils.getSolrConnection(cli); | ||
| echoIfVerbose("Connecting to Solr at " + solrConnection); | ||
|
|
||
| StreamContext streamContext = new StreamContext(); | ||
| streamContext.setSolrClientCache(solrClientCache); | ||
|
|
||
| StreamFactory streamFactory = new DefaultStreamFactory(); | ||
| streamFactory.withDefaultSolrConnection(solrConnection); | ||
| streamContext.setStreamFactory(streamFactory); | ||
| return streamContext; |
