jdyer1 commented on code in PR #2259:
URL: https://github.com/apache/solr/pull/2259#discussion_r1499996419
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -24,7 +24,8 @@
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.client.solrj.request.RequestWriter;
-public abstract class HttpSolrClientBuilderBase {
+public abstract class HttpSolrClientBuilderBase<
+ B extends HttpSolrClientBuilderBase<?, ?>, C extends Http2SolrClientBase> {
Review Comment:
I did remove the "2" from the name. However, any classes here I am naming
`*Base` were factored out of something specific to `Http2SolrClient`. I know
there might have been some copy/paste when the various other clients were
written but I did not check to see if we could create an abstract builder that
applies to all of the clients. That might be a worthy follow-up project though.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -75,9 +77,16 @@ protected HttpSolrClientJdkImpl(String serverBaseUrl,
HttpSolrClientJdkImpl.Buil
if (builder.sslContext != null) {
b.sslContext(builder.sslContext);
}
+
if (builder.executor != null) {
- b.executor(builder.executor);
+ this.executor = builder.executor;
+ this.shutdownExecutor = false;
+ } else {
+ this.executor =
Executors.newCachedThreadPool(Executors.defaultThreadFactory());
Review Comment:
I initially wanted to more-or-less mimic what
`java.net.util.http.HttpClient` does if you do not pass in an Executor, and
only reluctantly create my own because we need access to it for the pipes. But
deferring to `ForbiddenAPIs` I did switch it to the `ExecutorUtil` version
(sorry I accidently pushed WIP here).
I will mention here that I am a little concerned that `ExecutorUtil` has
some things in it that really do not belong to the client, that exist only for
inter-node requests. For this reason I was hoping to avoid it. See my other
PR for one case where this could possibly cause trouble.
https://github.com/apache/solr/pull/2242
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -36,34 +36,32 @@
import org.eclipse.jetty.util.ssl.SslContextFactory;
import org.junit.Test;
-public class Http2SolrClientTest extends
Http2SolrClientTestBase<Http2SolrClient.Builder> {
+public class Http2SolrClientTest extends Http2SolrClientTestBase {
@Override
protected String expectedUserAgent() {
return "Solr[" + Http2SolrClient.class.getName() + "] 2.0";
}
@Override
- protected <B extends HttpSolrClientBuilderBase> B builder(
- String url, int connectionTimeout, int socketTimeout, Class<B> type) {
Review Comment:
I will say that writing a generic builder in Java is no easy task! Maybe
I'm wrong on this but I do not think we can avoid unchecked casts here unless
we require the user to pass in the target class as an additional parameter on
each 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-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
