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