dsmiley commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1059527327
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java:
##########
@@ -140,15 +141,26 @@ protected ConcurrentUpdateSolrClient(Builder builder) {
}
}
+ /**
+ * @deprecated use {@link #getUrlParamNames()}
+ */
+ @Deprecated
Review Comment:
heh... honestly I doubt *anyone* calls getters, particularly this one, for
configuration options. I suspect someone added it "just because" and so here
it is. Just saying... do as you wish (leave or remove). Personally, I'd not
add a replacement.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -679,7 +680,7 @@ private Request createRequest(SolrRequest<?> solrRequest,
String collection)
contentWriter.getContentType(), ByteBuffer.wrap(baos.getbuf(),
0, baos.size())));
} else if (streams == null || isMultipart) {
// send server list and request list as query string params
- ModifiableSolrParams queryParams =
calculateQueryParams(this.queryParams, wparams);
+ ModifiableSolrParams queryParams =
calculateQueryParams(this.urlParamNames, wparams);
Review Comment:
OMG so much better after the rename :-). (this line referenced
`queryParams` twice in fundamentally different ways)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -1041,6 +1052,15 @@ public HttpSolrClient build() {
if
(this.invariantParams.get(DelegationTokenHttpSolrClient.DELEGATION_TOKEN_PARAM)
== null) {
return new HttpSolrClient(this);
} else {
+ urlParamNames =
+ urlParamNames == null
+ ? Set.of(DelegationTokenHttpSolrClient.DELEGATION_TOKEN_PARAM)
+ : urlParamNames;
+ if
(!urlParamNames.contains(DelegationTokenHttpSolrClient.DELEGATION_TOKEN_PARAM))
{
+ urlParamNames = new HashSet<>(urlParamNames);
+
urlParamNames.add(DelegationTokenHttpSolrClient.DELEGATION_TOKEN_PARAM);
+ urlParamNames = Collections.unmodifiableSet(urlParamNames);
+ }
return new DelegationTokenHttpSolrClient(this);
Review Comment:
```suggestion
}
urlParamNames = Set.copyOf(urlParamNames);
return new DelegationTokenHttpSolrClient(this);
```
The purpose here is to guarantee we have a truly immutable copy no matter
how it was populated. Note that Set.copyOf is smart -- it knows already purely
immutable copies and will do nothing if it sees such.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -82,25 +83,25 @@
* @since solr 8.0
*/
public class LBHttp2SolrClient extends LBSolrClient {
- private final Http2SolrClient httpClient;
+ private final Http2SolrClient http2SolrClient;
Review Comment:
I suggest `solrClient` instead -- it's shorter and it need not specify
`http2` in it.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
