iamsanjay commented on PR #2276:
URL: https://github.com/apache/solr/pull/2276#issuecomment-1955856284
I was thinking about the executor in the Http2SolrClient class.
**What is the issue?**
1. Http2SolrClent calls `createHttpClient` to create a Jetty HTTP client. By
default, If executor is not provided then a executor will be created for it to
initialize the Jetty HTTP.
```
private HttpClient createHttpClient(Builder builder) {
HttpClient httpClient;
executor = builder.executor;
if (executor == null) {
BlockingArrayQueue<Runnable> queue = new BlockingArrayQueue<>(256,
256);
this.executor =
new ExecutorUtil.MDCAwareThreadPoolExecutor(
32, 256, 60, TimeUnit.SECONDS, queue, new
SolrNamedThreadFactory("h2sc"));
shutdownExecutor = true;
} else {
shutdownExecutor = false;
}
```
2. However, If someone creates Http2SolrClient.Builder with existing Jetty
HTTP client by calling `withHttpClient` there will be no executor available to
them, even if `withExecutor` method is called. Therefore, all the asyncRequest
will result in NullPointerException. Because only `createHttpClient` is the
only method which handles the initialization of executor and If new builder is
created is using existing Jetty client then no call is made to
`createHttpClient` and hence no executor.
**Solution**
There are two ways that I am thinking as of now to handle it, in case If new
builder is created is using the existing Jetty HTTP client.
1. In Http2SolrClient constructor, below snipped of code is added to check
whether executor is null or not. And then re-initlialize the executor at
Http2SolrClient level. At this point, we have two executors: one for
Http2SolrClient and another for Jetty Http client. And Http2SolrClient is only
responsible for closing the ones they making from inside, and If you are
providing any Executor from outside, then user will be responsible for closing
it.
```
if (this.executor == null) {
this.executor = http2SolrClient.executor;
}
```
2. Second, As we recreate builder using existing Jetty Client by calling
`withHttpClient`, there we can also add code that would also set executor(one
which we created calling `createHttpClient`) for Http2SolrClient as well among
with other properties. But that means that Every builder recreated using the
existing Jetty client will have access to executor without them knowing about
it. Below is that executor. Something I am not much comfortable about, I
believe If user creating Http2SolrClient to send asyncRequest then they should
provide the implementation for executor.
Also In this option there will be code to check wh
```
BlockingArrayQueue<Runnable> queue = new BlockingArrayQueue<>(256, 256);
this.executor =
new ExecutorUtil.MDCAwareThreadPoolExecutor(
32, 256, 60, TimeUnit.SECONDS, queue, new
SolrNamedThreadFactory("h2sc"));
```
@dsmiley wdyt? I have changed the code to implement the second option for
now.
@stillalex I see that we transferred some properties from older
Http2SolrClient as we call `withHttpClient` to create new Http2SolrClient but
the exisitng executor was never used for the new builder. Was there any reason
for 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: [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]
