SOLR-17211: HttpJdkSolrClient Support Async requests #2374
Conversation
- "POST" test does not pass! (TODO)
There was a problem hiding this comment.
Overall, you did a thorough job; not much to add beyond what I said in the parent JIRA. I'm hoping you are sympathetic to that other JIRA and its existing PR so we can go a different route instead of cementing further Solr's needless bespoke Async API (AsyncListener & Cancellable).
| @@ -136,6 +137,16 @@ private void recordRequest(HttpServletRequest req, HttpServletResponse resp) { | |||
| resp.addHeader(h[0], h[1]); | |||
| } | |||
| } | |||
| String qs = req.getQueryString(); | |||
There was a problem hiding this comment.
could use a one-liner comment to explain
| } | ||
| } | ||
|
|
||
| protected DebugAsyncListener testAsyncExceptionBase() throws Exception { |
There was a problem hiding this comment.
Not sure if tests can return non-void. If so, you could just make it public and add @Test and remove the "Base" suffix in the name.
There was a problem hiding this comment.
Let me clarify why I have been overriding these shared tests. You are right it does work generally on IntelliJ, however, if you want to run only 1 test method and not the whole class, this doesn't seem to work. Even if you run all the tests, then right click to re-run just the individual test method, I am getting an error. This makes it very difficult to debug, which is why I have been using all those 1-line test override methods.
| @@ -469,6 +531,23 @@ protected String allProcessorSupportedContentTypesCommaDelimited( | |||
| .collect(Collectors.joining(", ")); | |||
| } | |||
|
|
|||
| protected static class HttpSolrClientCancellable implements Cancellable { | |||
There was a problem hiding this comment.
Seeing this class, to me, really underscores a mismatch between Solr's needless Async API helper classes (Cancellable is one), and using a CompletableFuture directly. See the parent JIRA issue where I direct you to a previous stalled effort that could be resumed.
There was a problem hiding this comment.
Yes I fully agree we should deprecate and replace the existing method, but we will need to keep the old API around for 9.x. I am interested in taking a closer look at SOLR-14763 and I appreciate your pointing me to that. But in any case I would like to do that separately from this issue.
There was a problem hiding this comment.
Okay. Note the old API is on exactly one class a user might use; it's being expanded here. But at least it's only other Http subclasses; isn't spreading further. If you can mark these methods Deprecated here (to be replaced by something coming (soon?)), people will be forewarned.
There was a problem hiding this comment.
I do not want to deprecate anything unless there is a replacement. But I hear and share your concern, and would like to make the API change, just not as part of this PR.
- could not use DebugServlet to do a thorough post test, switch to indexing actual solr documents
|
Unless there are objections I would like to merge this one soon. |
github-actions
bot
added
the
documentation
This PR adds method
asyncRequestto abstract supertypeHttpSolrClientBaseand implements it onHttpJdkSolrClient. Also adds unit tests for normal, exceptional and cancelled asynchronous requests, both forHttpJdkSolrClientandHttp2SolrClient. Also includes a few javadoc improvements.