dsmiley commented on code in PR #2259:
URL: https://github.com/apache/solr/pull/2259#discussion_r1506391902
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -108,6 +118,29 @@ protected HttpSolrJdkClient(String serverBaseUrl,
HttpSolrJdkClient.Builder buil
}
this.client = b.build();
updateDefaultMimeTypeForParser();
+
+ // This is a workaround for the case where the first request using
Review Comment:
Awesome comment/explanation.
Should this ping be gated on http2 only?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -64,7 +66,7 @@ public class HttpSolrJdkClient extends HttpSolrClientBase {
private static final String USER_AGENT =
"Solr[" + MethodHandles.lookup().lookupClass().getName() + "] 1.0";
- private HttpClient client;
+ protected HttpClient client;
Review Comment:
minor, but can we call this `httpClient` to make it clearer wherever we use
it what sort of client we're talking about?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -210,50 +243,50 @@ private HttpResponse<InputStream> doPutOrPost(
}
HttpRequest.BodyPublisher bodyPublisher;
- final PipedOutputStream source;
+ Future<?> contentWritingFuture = null;
if (contentWriter != null) {
- source = new PipedOutputStream();
- PipedInputStream sink = new PipedInputStream(source);
+
+ final PipedOutputStream source = new PipedOutputStream();
+ final PipedInputStream sink = new PipedInputStream(source);
bodyPublisher = HttpRequest.BodyPublishers.ofInputStream(() -> sink);
- // NOCOMMIT - this does not work. The call to "client.send" sometimes
will throw an
- // IOException
- // because our thread closes the outputstream too early. But if we wait
to close it until
- // after the response returns, "client.send" blocks.
- executor.submit(
- () -> {
- try (source) {
- contentWriter.write(source);
- } catch (Exception e) {
- log.error("Cannot write Content Stream", e);
- }
- });
+
+ contentWritingFuture =
+ executor.submit(
+ () -> {
+ try (source) {
+ contentWriter.write(source);
+ } catch (Exception e) {
+ log.error("Cannot write Content Stream", e);
+ }
+ });
} else if (streams != null && streams.size() == 1) {
- source = null;
InputStream is = streams.iterator().next().getStream();
bodyPublisher = HttpRequest.BodyPublishers.ofInputStream(() -> is);
} else if (queryParams != null && urlParamNames != null) {
- source = null;
ModifiableSolrParams requestParams = queryParams;
queryParams = calculateQueryParams(urlParamNames, requestParams);
queryParams.add(calculateQueryParams(solrRequest.getQueryParams(),
requestParams));
bodyPublisher =
HttpRequest.BodyPublishers.ofString(requestParams.toString());
} else {
- source = null;
bodyPublisher = HttpRequest.BodyPublishers.noBody();
}
+
if (isPut) {
reqb.PUT(bodyPublisher);
} else {
reqb.POST(bodyPublisher);
}
decorateRequest(reqb, solrRequest);
reqb.uri(new URI(url + "?" + queryParams));
- HttpResponse<InputStream> response =
- client.send(reqb.build(), HttpResponse.BodyHandlers.ofInputStream());
+
+ HttpResponse<InputStream> response;
try {
- source.close();
- } catch (Exception e1) {
- // ignore
+ response = client.send(reqb.build(),
HttpResponse.BodyHandlers.ofInputStream());
+ } catch (IOException | InterruptedException | RuntimeException e) {
+ if (contentWritingFuture != null) {
+ contentWritingFuture.cancel(true);
+ }
+ throw e;
}
Review Comment:
It's okay to cancel a task that has already completed. So we can simplify
this to move the logic to a finally. No need to catch exceptions.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrJdkClientTest.java:
##########
@@ -278,6 +281,15 @@ public void testUpdateDefault() throws Exception {
@Test
public void testUpdateXml() throws Exception {
+ testUpdateXml(false);
+ }
+
+ @Test
+ public void testUpdateXmlWithHttp11() throws Exception {
+ testUpdateXml(true);
+ }
Review Comment:
Could we parameterize the test instead so that the whole test runs in both
modes?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -38,7 +37,7 @@ public abstract class HttpSolrClientBuilderBase<
protected Set<String> urlParamNames;
protected Integer maxConnectionsPerHost;
protected ExecutorService executor;
- protected CookieStore cookieStore;
+ protected boolean useHttp1_1 = Boolean.getBoolean("solr.http1");
Review Comment:
I like it but I can't help but notice that no other field here has a default
that is defined here. So _maybe_ this new setting should be initialized
elsewhere?
--
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
