dsmiley commented on code in PR #3400:
URL: https://github.com/apache/solr/pull/3400#discussion_r2159689428
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -38,7 +38,8 @@ public abstract class HttpSolrClientBuilderBase<
protected Set<String> urlParamNames;
protected Integer maxConnectionsPerHost;
protected ExecutorService executor;
- protected boolean useHttp1_1 = Boolean.getBoolean("solr.http1");
+ protected final boolean defaultUseHttp1_1 = Boolean.getBoolean("solr.http1");
Review Comment:
this could be static (and thus moved above to statics); right?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -167,10 +168,26 @@ public long getRequestTimeoutMillis() {
*/
@SuppressWarnings("unchecked")
public B useHttp1_1(boolean useHttp1_1) {
- this.useHttp1_1 = useHttp1_1;
+ this.providedUseHttp1_1 = useHttp1_1;
return (B) this;
}
+ /**
+ * Return whether the HttpSolrClient built will prefer http1.1 over http2.
This will be determined
Review Comment:
I think this is saying too much honestly. Docs need to be maintained. It
exposes little details and possibly repeats things elsewhere. *every* getter
(lets say this is a getter), will use what is provided if the user supplies a
choice. Let's not say the obvious, since then we'll be tempted to repeat this
again and again and again...
Secondly, any getter like method (includes this) on a builder is really for
internal use, so I wouldn't bother documenting it. Thirdly, if you want to
document the semantics of what this settings means, the place to do that is on
the setter (or "with" method). It's already documented. So just drop this IMO.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -38,7 +38,8 @@ public abstract class HttpSolrClientBuilderBase<
protected Set<String> urlParamNames;
protected Integer maxConnectionsPerHost;
protected ExecutorService executor;
- protected boolean useHttp1_1 = Boolean.getBoolean("solr.http1");
+ protected final boolean defaultUseHttp1_1 = Boolean.getBoolean("solr.http1");
+ protected Boolean providedUseHttp1_1;
Review Comment:
personally, I'm not a fan of "provided". The vast majority of fields here
are in-effect "provided", and they don't need this name prefix.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -132,8 +132,9 @@ protected Http2SolrClient(String serverBaseUrl, Builder
builder) {
if (builder.followRedirects != null
|| builder.connectionTimeoutMillis != null
|| builder.maxConnectionsPerHost != null
- || builder.useHttp1_1
- != builder.httpClient.getTransport() instanceof
HttpClientTransportOverHTTP
+ || (builder.providedUseHttp1_1 != null
+ && builder.providedUseHttp1_1
+ != builder.httpClient.getTransport() instanceof
HttpClientTransportOverHTTP)
Review Comment:
I think we should not drop this part now -- just keep this looking _just_
like the other settings with null checks.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -167,10 +168,26 @@ public long getRequestTimeoutMillis() {
*/
@SuppressWarnings("unchecked")
public B useHttp1_1(boolean useHttp1_1) {
- this.useHttp1_1 = useHttp1_1;
+ this.providedUseHttp1_1 = useHttp1_1;
return (B) this;
}
+ /**
+ * Return whether the HttpSolrClient built will prefer http1.1 over http2.
This will be determined
+ * first if {@link #useHttp1_1(boolean)} is called, then by checking the
"solr.http1" System
+ * Property, and otherwise will default to false.
+ *
+ * @return whether the build HttpSolrClient will prefer http1.1
+ */
+ @SuppressWarnings("unchecked")
+ public boolean shouldUseHttp1_1() {
+ if (providedUseHttp1_1 != null) {
+ return providedUseHttp1_1;
+ } else {
+ return defaultUseHttp1_1;
+ }
Review Comment:
ternary please
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
