You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/12/28 13:21:14 UTC

[GitHub] [solr] epugh opened a new pull request, #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

epugh opened a new pull request, #1253:
URL: https://github.com/apache/solr/pull/1253

   https://issues.apache.org/jira/browse/SOLR-10452
   
   
   # Description
   
   setQueryParams should be deprecated in favor of SolrClientBuilder methods.
   
   # Solution
   
   I moved the the setQueryParams over for the Http2SolrClient, it already had been done for HttpSolrClient.   I noticed we have a `addQueryParams` method, which I marked deprecated and we shouldn't use, as that goes against the idea of a Solr Client being immutable.   
   
   One area I tried to fix and gave up on was the `DelegationTokenHttpSolrClient`, I couldn't quite figure out what to do there, would love a suggestion or a fix ;-)
   
   # Tests
   
   Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) 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)
   - [ ] 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


[GitHub] [solr] dsmiley commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058589620


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -325,9 +338,24 @@ public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval) {
       return this;
     }
 
+    /**
+     * Expert Method
+     *
+     * @param queryParams set of param keys that are only sent via the query string. Note that the
+     *     param will be sent as a query string if the key is part of this Set or the SolrRequest's
+     *     query params.
+     * @see org.apache.solr.client.solrj.SolrRequest#getQueryParams
+     */
+    public LBHttp2SolrClient.Builder withQueryParams(Set<String> queryParams) {

Review Comment:
   as you are showing willingness to change the name of fields in this PR (i.e. http2SolrClient), I would also really appreciate any field/parameter named "queryParams" to be something like "urlParamNames" so we are less likely to be confused at a glance with SolrParams.



-- 
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


[GitHub] [solr] dsmiley commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058586092


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -325,9 +338,24 @@ public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval) {
       return this;
     }
 
+    /**
+     * Expert Method
+     *
+     * @param queryParams set of param keys that are only sent via the query string. Note that the
+     *     param will be sent as a query string if the key is part of this Set or the SolrRequest's
+     *     query params.
+     * @see org.apache.solr.client.solrj.SolrRequest#getQueryParams
+     */
+    public LBHttp2SolrClient.Builder withQueryParams(Set<String> queryParams) {

Review Comment:
   in the spirit of the Builder reading like English more naturally, how about `withTheseParamNamesInTheUrl`.  Yes it's long but who cares -- this is some expert method.
   



-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366828996

   proposal for DelegationTokenHttpSolrClient: 
   * move constructor 'setQueryParams(' call to Builder itself. the call comes from the builder already, so no added complexity https://github.com/apache/solr/blob/0f912f6b84c6b0a42079df2479ac6bbecc3c2c67/solr/solrj/src/java/org/apache/solr/client/solrj/impl/DelegationTokenHttpSolrClient.java#L40
   * mark setQueryParams method deprecated
   I think this should be it. in the future we can remove the DelegationTokenHttpSolrClient#setQueryParams method and the query params will already have the expected param because the builder injected it at build time.


-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1367376695

   Okay, are we done?


-- 
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


[GitHub] [solr] epugh commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058561116


##########
solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java:
##########
@@ -138,19 +138,19 @@ public UpdateShardHandler(UpdateShardHandlerConfig cfg) {
         HttpClientUtil.createClient(
             clientParams, defaultConnectionManager, false, httpRequestExecutor);
 
+    Set<String> queryParams = new HashSet<>(2);
+    queryParams.add(DistributedUpdateProcessor.DISTRIB_FROM);
+    queryParams.add(DistributingUpdateProcessorFactory.DISTRIB_UPDATE_PARAM);

Review Comment:
   it can be!   I think there are a lot of places we could do that...



-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366894780

   > LBHttp2SolrClient
   
   Would you be interested in pushing up a commit to make this change to remove the method?  I'd lvoe to see 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-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


[GitHub] [solr] epugh commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058612968


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -128,7 +128,7 @@ public class Http2SolrClient extends SolrClient {
   private static final List<String> errPath = Arrays.asList("metadata", "error-class");
 
   private HttpClient httpClient;
-  private volatile Set<String> queryParams = Collections.emptySet();
+  protected Set<String> queryParams = Set.of();

Review Comment:
   Thanks for the sharp eyes.. yeah, I got moving too fast.



-- 
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


[GitHub] [solr] dsmiley commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058555293


##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -661,12 +661,13 @@ private void verifyServletState(Http2SolrClient client, SolrRequest<?> request)
   public void testQueryString() throws Exception {
 
     final String clientUrl = jetty.getBaseUrl().toString() + "/debug/foo";
-    try (Http2SolrClient client = getHttp2SolrClient(clientUrl)) {
+    UpdateRequest req = new UpdateRequest();
+    setReqParamsOf(req, "serverOnly", "notServer");

Review Comment:
   it doesn't matter but its clearer to place this after client creation instead of before.  This is consistent with tests below.



##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -325,9 +338,24 @@ public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval) {
       return this;
     }
 
+    /**
+     * Expert Method
+     *
+     * @param queryParams set of param keys that are only sent via the query string. Note that the
+     *     param will be sent as a query string if the key is part of this Set or the SolrRequest's
+     *     query params.
+     * @see org.apache.solr.client.solrj.SolrRequest#getQueryParams
+     */
+    public LBHttp2SolrClient.Builder withQueryParams(Set<String> queryParams) {

Review Comment:
   I didn't know about this thing before.  The javadocs are nice but I nonetheless find this confusing; I previously might have thought this was something like default SolrParams, but no.  The odd thing is that this doesn't hold the actual parameters, instead it's _names_ of parameters that only result in some change if the name-value is specified on the SolrRequest.    If this were `withUrlQueryParams` (my preference) or `withQueryStringParams`, it'd be clearer.   WDYT?



##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -991,6 +992,7 @@ public static class Builder {
     private ExecutorService executor;
     protected RequestWriter requestWriter;
     protected ResponseParser responseParser;
+    private volatile Set<String> queryParams = Collections.emptySet();

Review Comment:
   definitely not volatile in a Builder!
   Eric, I think you were merely copying the field definition in the client.  It's only volatile there because of the setters we are removing, in case two threads work with the client at the same time.



-- 
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


[GitHub] [solr] epugh commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058982050


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -128,7 +128,7 @@ public class Http2SolrClient extends SolrClient {
   private static final List<String> errPath = Arrays.asList("metadata", "error-class");
 
   private HttpClient httpClient;
-  private volatile Set<String> queryParams = Collections.emptySet();
+  protected volatile Set<String> urlParamNames = Set.of();

Review Comment:
   thank you!



-- 
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


[GitHub] [solr] dsmiley commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1059545466


##########
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:
   My intention is only to make it obvious reading the code that urlParamNames will be immutable in the end.  That's all.  



-- 
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


[GitHub] [solr] epugh commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058586645


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -325,9 +338,24 @@ public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval) {
       return this;
     }
 
+    /**
+     * Expert Method
+     *
+     * @param queryParams set of param keys that are only sent via the query string. Note that the
+     *     param will be sent as a query string if the key is part of this Set or the SolrRequest's
+     *     query params.
+     * @see org.apache.solr.client.solrj.SolrRequest#getQueryParams
+     */
+    public LBHttp2SolrClient.Builder withQueryParams(Set<String> queryParams) {

Review Comment:
   ROTFL...   But I can totally get behind that!  



-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366974464

   ok. I was not able to push to your git, not sure why, I don't think I have the rights. I pushed to my repo based on your branch. could you take a look and let me know if it looks ok? https://github.com/stillalex/solr/commit/0490ab420ff1d90536f3572fc7b17b194f0724c1


-- 
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


[GitHub] [solr] stillalex commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058496938


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -991,6 +992,7 @@ public static class Builder {
     private ExecutorService executor;
     protected RequestWriter requestWriter;
     protected ResponseParser responseParser;
+    private volatile Set<String> queryParams = Collections.emptySet();

Review Comment:
   I don't think this needs to be 'volatile' anymore. also what about using 'Set.of()'?



-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366896499

   > * setQueryParams
   
   Likewise @stillalex I think I get what you are suggesting, you want to push up a change for this?  


-- 
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


[GitHub] [solr] dsmiley commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
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-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


[GitHub] [solr] stillalex commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058496938


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -991,6 +992,7 @@ public static class Builder {
     private ExecutorService executor;
     protected RequestWriter requestWriter;
     protected ResponseParser responseParser;
+    private volatile Set<String> queryParams = Collections.emptySet();

Review Comment:
   I don't think this needs to be 'volatile' anymore



-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366899956

   Okay, I think I've dealt with the small changes.   @stillalex I'd love you to push up changes for the two suggestions you had, they sound great.   @dsmiley maybe you can weigh in on the name of the field, and I'll make that change, and then this should be ready to merge!   Thanks for the feedback.


-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366975670

   odd, cause I think you have permissiosn to apache/solr repo?   Can you check out the PR from apache/solr instead of my branch from epugh/solr, and then push up changes????   


-- 
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


[GitHub] [solr] epugh commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058565251


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -325,9 +338,24 @@ public LBHttp2SolrClient.Builder setAliveCheckInterval(int aliveCheckInterval) {
       return this;
     }
 
+    /**
+     * Expert Method
+     *
+     * @param queryParams set of param keys that are only sent via the query string. Note that the
+     *     param will be sent as a query string if the key is part of this Set or the SolrRequest's
+     *     query params.
+     * @see org.apache.solr.client.solrj.SolrRequest#getQueryParams
+     */
+    public LBHttp2SolrClient.Builder withQueryParams(Set<String> queryParams) {

Review Comment:
   Yeah, I don't like the name either, and the confusion is real, witness the test that had `key=value` being used as query param!  instead of just `key`.    Any thoguhts on a name that really focuses on the fact that it's the key?   `withUrlQueryKeys` ???    or `withQueryKeys` ?



-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366932660

   > > * setQueryParams
   > 
   > Likewise @stillalex I think I get what you are suggesting, you want to push up a change for this?
   
   sure. how should we do this? push a proposed change to your branch directly for review?


-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1367017948

   @stillalex I learned somethign new!  added you as another repo, and was able to cherry-pick your commit!


-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1367037337

   I've seen you only partially took changes from https://github.com/stillalex/solr/commit/0490ab420ff1d90536f3572fc7b17b194f0724c1 was there anything failing? or was the change too drastic?


-- 
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


[GitHub] [solr] epugh commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058561752


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -991,6 +992,7 @@ public static class Builder {
     private ExecutorService executor;
     protected RequestWriter requestWriter;
     protected ResponseParser responseParser;
+    private volatile Set<String> queryParams = Collections.emptySet();

Review Comment:
   we should make this set.of everywehre ;-)  And prevent the Collections.emptySet via forbidden apis...



-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366973016

   > > > > * setQueryParams
   > > > 
   > > > 
   > > > Likewise @stillalex I think I get what you are suggesting, you want to push up a change for this?
   > > 
   > > 
   > > sure. how should we do this? push a proposed change to your branch directly for review?
   > 
   > Yeah, I think you can checkout my PR, or maybe my `epugh/solr` repo? Honestly, my Git Fu isn't great! I am always very happy to have other folks directly contribute to my branches ;-) Casue generally I am bumbling along!
   
   With small concrete commits, I can always revert something ;-)


-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366978519

   > odd, cause I think you have permissiosn to apache/solr repo? Can you check out the PR from apache/solr instead of my branch from epugh/solr, and then push up changes????
   
   I don't, I'm not a Solr committer :)
   
   


-- 
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


[GitHub] [solr] stillalex commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058609508


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -128,7 +128,7 @@ public class Http2SolrClient extends SolrClient {
   private static final List<String> errPath = Arrays.asList("metadata", "error-class");
 
   private HttpClient httpClient;
-  private volatile Set<String> queryParams = Collections.emptySet();
+  protected Set<String> queryParams = Set.of();

Review Comment:
   I think this should remain 'volatile', see @dsmiley's comments on the builder's use of volatile.



-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366778424

   tests all pass!


-- 
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


[GitHub] [solr] stillalex commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058956658


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -128,7 +128,7 @@ public class Http2SolrClient extends SolrClient {
   private static final List<String> errPath = Arrays.asList("metadata", "error-class");
 
   private HttpClient httpClient;
-  private volatile Set<String> queryParams = Collections.emptySet();
+  protected volatile Set<String> urlParamNames = Set.of();

Review Comment:
   @epugh bumping this comment up for visibility



-- 
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


[GitHub] [solr] stillalex commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058496493


##########
solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java:
##########
@@ -138,19 +138,19 @@ public UpdateShardHandler(UpdateShardHandlerConfig cfg) {
         HttpClientUtil.createClient(
             clientParams, defaultConnectionManager, false, httpRequestExecutor);
 
+    Set<String> queryParams = new HashSet<>(2);
+    queryParams.add(DistributedUpdateProcessor.DISTRIB_FROM);
+    queryParams.add(DistributingUpdateProcessorFactory.DISTRIB_UPDATE_PARAM);

Review Comment:
   Could this block be queryParams = Set.of(...);



-- 
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


[GitHub] [solr] epugh merged pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh merged PR #1253:
URL: https://github.com/apache/solr/pull/1253


-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1367273190

   > I've seen you only partially took changes from [stillalex@0490ab4](https://github.com/stillalex/solr/commit/0490ab420ff1d90536f3572fc7b17b194f0724c1) was there anything failing? or was the change too drastic?
   
   Huh...   I don't know why my cherry pick of that commit didn't bring it all?   I am going to manually review and copy over now ;-(


-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1367381679

   yep, looks good to me!


-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366649214

   Hey @stillalex would love a review.


-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366816568

   I find LBHttp2SolrClient's use of queryParams a bit confusing (it seems to be on LBSolrClient level), apologies if this is just my incorrect understanding. Why does the LBSolrClient need to maintain a separate set of query params? LBHttp2SolrClient already receives a Http2SolrClient which ideally would have been init with correct query params. any (now deprecated) method to modify query params would just delegate to Http2SolrClient instance. I would cautiously suggest to remove the extra set of query params, which would also simplify a bit the code.


-- 
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


[GitHub] [solr] epugh commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366972806

   > > > * setQueryParams
   > > 
   > > 
   > > Likewise @stillalex I think I get what you are suggesting, you want to push up a change for this?
   > 
   > sure. how should we do this? push a proposed change to your branch directly for review?
   
   Yeah, I think you can checkout my PR, or maybe my `epugh/solr` repo?  Honestly, my Git Fu isn't great!   I am always very  happy to have other folks directly contribute to my branches ;-)   Casue generally I am bumbling along!


-- 
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


[GitHub] [solr] stillalex commented on pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1253:
URL: https://github.com/apache/solr/pull/1253#issuecomment-1366979347

   suggested fix for the DelegationTokenHttpSolrClient https://github.com/stillalex/solr/commit/5086e0fe099d5c2fa5942c6b3f18b7d803370136 . 
   
   The constructor code looks a bit iffy to me. does it really overwrite any existing param from the builder to set this single param? it can't be correct.
   ```
   setQueryParams(new TreeSet<>(Arrays.asList(DELEGATION_TOKEN_PARAM)));
   ```
   


-- 
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


[GitHub] [solr] stillalex commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1058699692


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -128,7 +128,7 @@ public class Http2SolrClient extends SolrClient {
   private static final List<String> errPath = Arrays.asList("metadata", "error-class");
 
   private HttpClient httpClient;
-  private volatile Set<String> queryParams = Collections.emptySet();
+  protected volatile Set<String> urlParamNames = Set.of();

Review Comment:
   I don't think this needs to be protected anymore (following latest changes)



-- 
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


[GitHub] [solr] stillalex commented on a diff in pull request #1253: SOLR-10452: setQueryParams should be deprecated in favor of SolrClientBuilder methods

Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1253:
URL: https://github.com/apache/solr/pull/1253#discussion_r1059544372


##########
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:
   I don't disagree, and I like the Set methods more. but if you look at the impl, the UnmodifiableCollection is only a thin wrapper over the original collection, so I'm not sure this type of optimizations is really that useful in the end.



-- 
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