dsmiley commented on code in PR #4655:
URL: https://github.com/apache/solr/pull/4655#discussion_r3785959672
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -78,65 +79,74 @@ public class HttpJdkSolrClient extends HttpSolrClient {
protected HttpClient httpClient;
+ /**
+ * Executor used to stream (produce) request bodies into the pipe consumed
by the JDK HttpClient.
+ * This is the "producer" side and may be supplied by the caller.
+ */
+ protected ExecutorService requestBodyExecutor;
+
+ /** Dedicated executor handed to the JDK HttpClient */
protected ExecutorService executor;
private boolean forceHttp11;
private final boolean shutdownExecutor;
+ /**
+ * {@link ExecutorService} on {@link HttpJdkSolrClient.Builder} is used for
{@link HttpClient} only.
+ */
protected HttpJdkSolrClient(String serverBaseUrl, HttpJdkSolrClient.Builder
builder) {
super(serverBaseUrl, builder);
- HttpClient.Builder b = HttpClient.newBuilder();
+ HttpClient.Builder httpClientBuilder = HttpClient.newBuilder();
HttpClient.Redirect followRedirects =
Boolean.TRUE.equals(builder.getFollowRedirects())
? HttpClient.Redirect.NORMAL
: HttpClient.Redirect.NEVER;
- b.followRedirects(followRedirects);
+ httpClientBuilder.followRedirects(followRedirects);
- b.connectTimeout(Duration.of(builder.getConnectionTimeoutMillis(),
ChronoUnit.MILLIS));
+ httpClientBuilder.connectTimeout(
+ Duration.of(builder.getConnectionTimeoutMillis(), ChronoUnit.MILLIS));
// note: idle timeout isn't used for the JDK client
// note: request timeout is set per request
if (builder.sslContext != null) {
- b.sslContext(builder.sslContext);
+ httpClientBuilder.sslContext(builder.sslContext);
}
if (builder.getExecutor() != null) {
this.executor = builder.getExecutor();
this.shutdownExecutor = false;
} else {
- BlockingQueue<Runnable> queue = new LinkedBlockingQueue<>(1024);
this.executor =
- new ExecutorUtil.MDCAwareThreadPoolExecutor(
- 4,
- 256,
- 60,
- TimeUnit.SECONDS,
- queue,
- new SolrNamedThreadFactory(this.getClass().getSimpleName()));
+ ExecutorUtil.newMDCAwareCachedThreadPool(
+ new SolrNamedThreadFactory(this.getClass().getSimpleName() +
"-http"));
Review Comment:
I would prefer to restore the name; no "-http" suffix. The class is for
HTTP! (i.e. it doesn't disambiguate). As this is the natural/default executor,
I don't think it needs disambiguation.
##########
solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc:
##########
@@ -95,6 +95,9 @@ When enabled, this escape hatch relaxes both the login-time
check and the user c
It is intended as a temporary measure while you migrate the affected accounts
to stronger passwords, and should be removed once that is done.
=== SolrJ
+`HttpJdkSolrClient` now uses two separate ThreadPoolExecutors, one for the JDK
HttpClient (consumer) and one for writing request bodies (producer).
+Both are unbounded cached thread pools (maximumPoolSize = Integer.MAX_VALUE),
created with ExecutorUtil.newMDCAwareCachedThreadPool.
+If a Builder.executor is provided, it will be used for the HttpClient only.
The one for writing the request bodies cannot be passed in.
Review Comment:
```suggestion
The `HttpJdkSolrClient` no longer has default thread/connection limits, at
least not beyond what the JDK's client intrinsically does.
```
I think this is the only aspect worth putting in this file. Executors is
minutia.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -78,65 +79,74 @@ public class HttpJdkSolrClient extends HttpSolrClient {
protected HttpClient httpClient;
+ /**
+ * Executor used to stream (produce) request bodies into the pipe consumed
by the JDK HttpClient.
+ * This is the "producer" side and may be supplied by the caller.
Review Comment:
Oh? Where? FWIW I don't think we should add to the builder over this minor
matter.
--
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]
