SOLR-16190: Http2SolrClient should make sure to shutdown the executor#848
SOLR-16190: Http2SolrClient should make sure to shutdown the executor#848risdenk merged 3 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
I have no idea if destroy() is needed, but I was surprised it existed. When I stepped through with the debugger, destroy cleaned up more resources which seems to be a good idea on close()...
There was a problem hiding this comment.
https://www.eclipse.org/jetty/javadoc/jetty-9/org/eclipse/jetty/client/HttpClient.html is Destroyable (https://www.eclipse.org/jetty/javadoc/jetty-9/org/eclipse/jetty/util/component/Destroyable.html)
Typically a Destroyable is a LifeCycle component that can hold onto resources over multiple start/stop cycles. A call to destroy will release all resources and will prevent any further start/stop cycles from being successful.
So I think we actually want to destroy the httpClient here since we can't start the HttpClient again anyway...
There was a problem hiding this comment.
https://github.com/apache/solr/blob/main/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java#L242 is the only place we call HttpClient#start() so we should call stop and destroy when we are done.
There was a problem hiding this comment.
Makes sense to call destroy to me.
There was a problem hiding this comment.
Make sure we shutdown the executor even if the close client fails - do it in the finally block.
| try { | ||
| httpClient.start(); | ||
| } catch (Exception e) { | ||
| close(); // make sure we clean up |
There was a problem hiding this comment.
Make sure we cleanup on a failed start. At least the executor could have been created at this point. Other things might have been created as well.
|
@madrob / @magibney / @CaoManhDat any thoughts on this? |
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
+1 looks right. Thanks Kevin.
There was a problem hiding this comment.
Makes sense to call destroy to me.
2 participants
https://issues.apache.org/jira/browse/SOLR-16190