dsmiley commented on a change in pull request #1688:
URL: https://github.com/apache/lucene-solr/pull/1688#discussion_r463282674
##########
File path:
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
##########
@@ -102,6 +103,8 @@
private static CloudHttp2SolrClient httpBasedCloudSolrClient = null;
private static CloudHttp2SolrClient zkBasedCloudSolrClient = null;
+ private boolean asyncCallbackRun;
Review comment:
Don't declare mutable state on a test that could better be placed within
a test method (reduction in scope). Furthermore, in doing so it avoids the
need to reset it between tests, which you didn't do.
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseCloudSolrClient.java
##########
@@ -1040,7 +1055,7 @@ public RouteException(ErrorCode errorCode,
NamedList<Throwable> throwables, Map<
return resp;
}
- protected NamedList<Object>
sendRequest(@SuppressWarnings({"rawtypes"})SolrRequest request, List<String>
inputCollections)
+ protected NamedList<Object>
sendRequest(@SuppressWarnings({"rawtypes"})SolrRequest request, List<String>
inputCollections, AsyncListener<LBSolrClient.Rsp> asyncListener)
Review comment:
Can use `SolrRequest<?> request` instead of suppressing the warning.
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java
##########
@@ -113,6 +118,25 @@ protected boolean wasCommError(Throwable rootCause) {
return false;
}
+ /**
+ * Execute an asynchronous request against a Solr server for a given
collection
+ *
+ * @param request the request to execute
+ * @param collection the collection to execute the request against
+ * @param asyncListener the request listener, not null (use a no-op listener
instead for async requests with no callback)
+ *
+ * @return a {@link Cancellable} allowing you to cancel execution of the
async request
+ *
+ * @throws IOException If there is a low-level I/O error.
+ * @throws SolrServerException if there is an error on the server
+ */
+ public Cancellable asyncRequest(@SuppressWarnings({"rawtypes"}) SolrRequest
request,
Review comment:
Can use `SolrRequest<?> request` instead of suppressing the warning.
I'm concerned about LBSolrClient.Rsp being part of the API signature here.
It seems rather internal, even though it's public. Instead, can't you pass the
`NamedList<Object> rsp` part of the Rsp?
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java
##########
@@ -113,6 +118,25 @@ protected boolean wasCommError(Throwable rootCause) {
return false;
}
+ /**
+ * Execute an asynchronous request against a Solr server for a given
collection
+ *
+ * @param request the request to execute
+ * @param collection the collection to execute the request against
+ * @param asyncListener the request listener, not null (use a no-op listener
instead for async requests with no callback)
+ *
+ * @return a {@link Cancellable} allowing you to cancel execution of the
async request
+ *
+ * @throws IOException If there is a low-level I/O error.
+ * @throws SolrServerException if there is an error on the server
+ */
+ public Cancellable asyncRequest(@SuppressWarnings({"rawtypes"}) SolrRequest
request,
Review comment:
Maybe instead of returning Cancellable and doing the hacks to make that
work, and instead of the caller passing AsyncListener, perhaps the callback
argument should be a `CompletableFuture<NamedList<Object>>` ? This has a
cancel method, and allows a single type to handle multiple use-cases that is
also likely to be known to callers than yet more Solr-isms.
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java
##########
@@ -113,6 +118,25 @@ protected boolean wasCommError(Throwable rootCause) {
return false;
}
+ /**
+ * Execute an asynchronous request against a Solr server for a given
collection
+ *
+ * @param request the request to execute
+ * @param collection the collection to execute the request against
+ * @param asyncListener the request listener, not null (use a no-op listener
instead for async requests with no callback)
+ *
+ * @return a {@link Cancellable} allowing you to cancel execution of the
async request
+ *
+ * @throws IOException If there is a low-level I/O error.
+ * @throws SolrServerException if there is an error on the server
+ */
+ public Cancellable asyncRequest(@SuppressWarnings({"rawtypes"}) SolrRequest
request,
Review comment:
I'm curious what you think @CaoManhDat
##########
File path:
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
##########
@@ -1074,4 +1077,37 @@ private void
queryWithPreferReplicaTypes(CloudHttp2SolrClient cloudClient,
}
}
+ /**
+ * Tests the async request code pathway to ensure that normal requests will
trigger the onSuccess callback.
+ */
+ @Test
+ public void asyncRequestTest() throws Exception {
+ final String collectionName = "asyncRequestCollection";
+
+ // Create collection
+ CollectionAdminRequest.createCollection(collectionName, "conf", 2,
1).process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(collectionName, 2, 2);
+
+ AsyncListener<LBHttp2SolrClient.Rsp> callback = new AsyncListener<>() {
+ @Override
+ public void onSuccess(LBHttp2SolrClient.Rsp rsp) { asyncCallbackRun =
true; }
+
+ @Override
+ public void onFailure(Throwable throwable) { }
+ };
+
+ QueryRequest request = new QueryRequest(); // params of the request aren't
important for this test
+
+ // Test success callback runs on a "successful" request
+ asyncCallbackRun = false;
+ getRandomClient().asyncRequest(request, collectionName, callback);
+
+ // Wait for up to 500ms timeout success callback to run
+ int count = 0;
+ while (!asyncCallbackRun && ++count <= 20) {
+ Thread.sleep(25);
Review comment:
Thread.sleep is slowly becoming blasphemy in Solr, including tests.
Avoid it, please. Instead, use a concurrent utility like Semaphore. I bet the
code will be easier to read too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
