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