dsmiley commented on code in PR #1211:
URL: https://github.com/apache/solr/pull/1211#discussion_r1038808801
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -132,6 +141,8 @@ public static class Builder {
protected boolean parallelUpdates = true;
protected ClusterStateProvider stateProvider;
protected Http2SolrClient.Builder internalClientBuilder;
+ private RequestWriter requestWriter;
+ private ResponseParser responseParser;
Review Comment:
In your commit message & CHANGES.txt, call this out explicitly. e.g.:
CloudHttp2SolrClient.Builder now has setters for the request writer &
responseParser.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java:
##########
@@ -28,8 +30,12 @@
protected HttpClient httpClient;
protected ResponseParser responseParser;
+ protected RequestWriter requestWriter;
+ protected boolean useMultiPartPost;
protected Integer connectionTimeoutMillis = 15000;
protected Integer socketTimeoutMillis = 120000;
+ protected boolean followRedirects = false;
+ protected Set<String> queryParams;
Review Comment:
more stuff to explicitly call out in CHANGES.txt
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestBatchUpdate.java:
##########
@@ -46,61 +46,68 @@ public static void beforeTest() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
- client.deleteByQuery("*:*"); // delete everything!
- doIt(client);
+ try (SolrClient client =
+ new HttpSolrClient.Builder(getServerUrl()).withRequestWriter(new
RequestWriter()).build()) {
+ client.deleteByQuery("*:*"); // delete everything!
+ doIt(client);
+ }
Review Comment:
Hmm; I'm wondering why this code was specifying HttpSolrClient and why was
it specifying a new RequestWriter. Do you know? Preferably it would do
neither and continue to call getSolrClient vs your change to create a new one.
##########
solr/core/src/test/org/apache/solr/request/TestRemoteStreaming.java:
##########
@@ -116,8 +116,10 @@ public void testNoUrlAccess() throws Exception {
SolrQuery query = new SolrQuery();
query.setQuery("*:*"); // for anything
query.add("stream.url", makeDeleteAllUrl());
- SolrException se = expectThrows(SolrException.class, () ->
getSolrClient().query(query));
- assertSame(ErrorCode.BAD_REQUEST, ErrorCode.getErrorCode(se.code()));
+ try (SolrClient solrClient = getHttpSolrClient(getServerUrl())) {
Review Comment:
What was wrong with it before? Now it depends on Http.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleBinaryTest.java:
##########
@@ -32,19 +35,13 @@ public static void beforeTest() throws Exception {
@Override
public SolrClient createNewSolrClient() {
- try {
- // setup the server...
- String url = jetty.getBaseUrl().toString() + "/collection1";
- HttpSolrClient client = getHttpSolrClient(url,
DEFAULT_CONNECTION_TIMEOUT);
- client.setUseMultiPartPost(random().nextBoolean());
+ HttpSolrClient.Builder httpSolrClientBuilder = new
HttpSolrClient.Builder(getServerUrl());
Review Comment:
minor: I don't think you need to declare any variables in this method; just
a one-liner to return. Variables need names, add verbosity, lines of code etc.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -258,4 +299,32 @@ public void onFailure(Throwable oe) {
}
});
}
+
+ public static class Builder {
+ private final Http2SolrClient http2Client;
+ private final String[] baseSolrUrls;
+ private ResponseParser responseParser;
+ private RequestWriter requestWriter;
+
+ public Builder(Http2SolrClient http2Client, String... baseSolrUrls) {
+ this.http2Client = http2Client;
+ this.baseSolrUrls = baseSolrUrls;
+ }
+
+ /** Provides a {@link ResponseParser} for created clients to use when
handling requests. */
+ public LBHttp2SolrClient.Builder withResponseParser(ResponseParser
responseParser) {
+ this.responseParser = responseParser;
+ return this;
+ }
+
+ /** Provides a {@link RequestWriter} for created clients to use when
handing requests. */
+ public LBHttp2SolrClient.Builder withRequestWriter(RequestWriter
requestWriter) {
+ this.requestWriter = requestWriter;
+ return this;
+ }
Review Comment:
Question: do we actually use response parsers/writers that are *different*
on this LB instance than that of the Http2SolrClient it delegates to?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -120,72 +112,22 @@ public void testUtf8PerfDegradation() throws Exception {
doc.addField("id", "1");
doc.addField("b_is", IntStream.range(0,
30000).boxed().collect(Collectors.toList()));
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.add(doc);
- client.commit();
- long start = System.nanoTime();
- QueryResponse rsp = client.query(new SolrQuery("*:*"));
- System.out.println("time taken : " + ((System.nanoTime() - start)) / (1000
* 1000));
- assertEquals(1, rsp.getResults().getNumFound());
- }
-
- @Ignore
- public void testUtf8QueryPerf() throws Exception {
Review Comment:
deleted because?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleBinaryHttp2Test.java:
##########
@@ -34,19 +37,15 @@ public static void beforeTest() throws Exception {
@Override
public SolrClient createNewSolrClient() {
- try {
- // setup the server...
- String url = jetty.getBaseUrl().toString() + "/collection1";
- Http2SolrClient client =
- new
Http2SolrClient.Builder(url).connectionTimeout(DEFAULT_CONNECTION_TIMEOUT).build();
-
- // where the magic happens
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+ String url = jetty.getBaseUrl().toString() + "/collection1";
+ Http2SolrClient client =
+ new Http2SolrClient.Builder(url)
+ .connectionTimeout(DEFAULT_CONNECTION_TIMEOUT)
+ .withRequestWriter(new BinaryRequestWriter())
+ // where the magic happens
+ .withResponseParser(new BinaryResponseParser())
+ .build();
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
- }
+ return client;
Review Comment:
super minor but it's no longer necessary to declare a field and separately
return it. Return directly -- less typing, one less variable to name :-)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -62,6 +64,12 @@ protected CloudHttp2SolrClient(Builder builder) {
this.clientIsInternal = false;
this.myClient = builder.httpClient;
}
+ if (builder.requestWriter != null) {
+ this.myClient.setRequestWriter(builder.requestWriter); // should be part
of builder
Review Comment:
What do you mean by "should be part of builder" -- it *is* (thanks to you);
no?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java:
##########
@@ -854,6 +858,7 @@ public static class Builder extends
SolrClientBuilder<Builder> {
protected String baseSolrUrl;
protected int queueSize = 10;
protected int threadCount;
+ protected int pollQueueTime;
Review Comment:
again, lets call this out in CHANGES.txt
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -476,9 +484,20 @@ public void testUpdate() throws Exception {
// parameter encoding
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ DebugServlet.clear();
+ // XML response and writer
+ try (HttpSolrClient client =
+ new HttpSolrClient.Builder(url)
+ .withRequestWriter(new RequestWriter())
+ .withResponseParser(new XMLResponseParser())
+ .build()) {
+ UpdateRequest req = new UpdateRequest();
+ req.add(new SolrInputDocument());
+ req.setParam("a", "\u1234");
- // XML response and writer
- client.setParser(new XMLResponseParser());
+ // Have not been able to move this to the Builder pattern, the below
check for application/xml
+ // always returns as application/javabin when .setRequestWriter() is
commented out.
Review Comment:
BTW needs to be called out as a "nocommit" (for discussion; not necessarily
stays this way) so we draw attention to this in a big PR
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestSolrJErrorHandling.java:
##########
@@ -102,18 +102,23 @@ public void showExceptions() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
- client.deleteByQuery("*:*"); // delete everything!
- doIt(client);
+ try (SolrClient client =
Review Comment:
same as my last comment
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrSchemalessExampleTest.java:
##########
@@ -132,20 +132,14 @@ public void testFieldMutating() throws Exception {
@Override
public SolrClient createNewSolrClient() {
- try {
- // setup the server...
- String url = jetty.getBaseUrl().toString() + "/collection1";
- HttpSolrClient client = getHttpSolrClient(url,
DEFAULT_CONNECTION_TIMEOUT);
- client.setUseMultiPartPost(random().nextBoolean());
-
- if (random().nextBoolean()) {
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
- }
-
- return client;
- } catch (Exception ex) {
- throw new RuntimeException(ex);
+ HttpSolrClient.Builder httpSolrClientBuilder = new
HttpSolrClient.Builder(getServerUrl());
Review Comment:
getServerUrl I assume doesn't have "/collection1" in it, or at least
shouldn't?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -174,11 +174,18 @@ protected Http2SolrClient(String serverBaseUrl, Builder
builder) {
} else {
basicAuthAuthorizationStr = null;
}
+ if (builder.requestWriter != null) {
Review Comment:
again, lets call this out in CHANGES.txt
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -599,25 +599,9 @@ public void testUnicode() throws Exception {
Random random = random();
int numIterations = atLeast(3);
- SolrClient client = getSolrClient();
-
- // save the old parser, so we can set it back.
- ResponseParser oldParser = null;
- if (client instanceof HttpSolrClient) {
- HttpSolrClient httpSolrClient = (HttpSolrClient) client;
- oldParser = httpSolrClient.getParser();
- }
+ try (SolrClient client = getSolrClient()) {
- try {
for (int iteration = 0; iteration < numIterations; iteration++) {
- // choose format
- if (client instanceof HttpSolrClient) {
- if (random.nextBoolean()) {
- ((HttpSolrClient) client).setParser(new BinaryResponseParser());
- } else {
- ((HttpSolrClient) client).setParser(new XMLResponseParser());
- }
Review Comment:
So we're losing randomized testing of some things? Okay but just confirming
this loss was deliberate because...?
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2871,10 +2871,30 @@ public static LBHttpSolrClient
getLBHttpSolrClient(String... solrUrls)
return new LBHttpSolrClient.Builder().withBaseSolrUrls(solrUrls).build();
}
+ /** This method creates a HttpClient from a URL. */
+ public static HttpClient getHttpClient(String url) {
+ return new HttpSolrClient.Builder(url).build().getHttpClient();
+ }
+
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting
SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
- * org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
+ * This method creates a basic HttpSolrClient. Tests that want to control
the creation process
+ * should use the {@link
org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
+ */
Review Comment:
again; deprecated and suggest clients simply *don't* use Apache HttpClient?
##########
solr/test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:
##########
@@ -534,16 +534,15 @@ public SortedMap<Class<? extends Filter>, String>
getExtraRequestFilters() {
}
protected SolrClient createNewSolrClient(int port) {
- try {
- // setup the client...
- String baseUrl = buildUrl(port);
- if (baseUrl.endsWith("/")) {
- return getHttpSolrClient(baseUrl + DEFAULT_TEST_CORENAME);
- } else {
- return getHttpSolrClient(baseUrl + "/" + DEFAULT_TEST_CORENAME);
- }
- } catch (Exception ex) {
- throw new RuntimeException(ex);
+ return getHttpSolrClient(getServerUrl(port));
+ }
+
+ protected String getServerUrl(int port) {
Review Comment:
Can we please call this getSolrWithCollectionUrl or something like that to
indicate it contains a collection in there? The word "serverUrl" by itself
suggests it points to the root of a Solr node, not to a particular collection
there.
##########
solr/test-framework/src/java/org/apache/solr/SolrJettyTestBase.java:
##########
@@ -129,6 +129,10 @@ public static JettySolrRunner createAndStartJetty(
return jetty;
}
+ protected String getServerUrl() {
Review Comment:
Same as last comment
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2886,28 +2906,25 @@ public static HttpSolrClient getHttpSolrClient(
}
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting
SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
- * org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
+ * This method creates a basic HttpSolrClient. Tests that want to control
the creation process
+ * should use the {@link
org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
*/
public static HttpSolrClient getHttpSolrClient(
String url, HttpClient httpClient, ResponseParser responseParser) {
return new
Builder(url).withHttpClient(httpClient).withResponseParser(responseParser).build();
}
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting
SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
- * org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
+ * This method creates a basic HttpSolrClient. Tests that want to control
the creation process
+ * should use the {@link
org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
*/
public static HttpSolrClient getHttpSolrClient(String url, HttpClient
httpClient) {
return new Builder(url).withHttpClient(httpClient).build();
}
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting
SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
- * org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
+ * This method creates a basic HttpSolrClient. Tests that want to control
the creation process
+ * should use the {@link
org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
Review Comment:
Did we discuss this overall change of direction away from randomization? I
brought this in once so I'm biased :-)
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2871,10 +2871,30 @@ public static LBHttpSolrClient
getLBHttpSolrClient(String... solrUrls)
return new LBHttpSolrClient.Builder().withBaseSolrUrls(solrUrls).build();
}
+ /** This method creates a HttpClient from a URL. */
+ public static HttpClient getHttpClient(String url) {
+ return new HttpSolrClient.Builder(url).build().getHttpClient();
+ }
Review Comment:
The direction our project is going in is to move away from Apache
HttpClient. Here you add a new utility method on SolrTestCaseJ4. Maybe mark
this deprecated?
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2871,10 +2871,30 @@ public static LBHttpSolrClient
getLBHttpSolrClient(String... solrUrls)
return new LBHttpSolrClient.Builder().withBaseSolrUrls(solrUrls).build();
}
+ /** This method creates a HttpClient from a URL. */
+ public static HttpClient getHttpClient(String url) {
+ return new HttpSolrClient.Builder(url).build().getHttpClient();
+ }
+
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting
SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
- * org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
+ * This method creates a basic HttpSolrClient. Tests that want to control
the creation process
+ * should use the {@link
org.apache.solr.client.solrj.impl.HttpSolrClient.Builder} class directly
+ */
+ public static HttpSolrClient getHttp1SolrClient(String url) {
+ return new HttpSolrClient.Builder(url).build();
+ }
+
+ /**
+ * This method creates a basic Http2SolrClient. Tests that want to control
the creation process
Review Comment:
There's a Javadoc style guide I once saw where, in this first sentence,
avoid saying "this method" or "this class". get immediately to the point,
probably with a verb "Creates ...". And avoid being obvious, as this is. this
creates a clients based on Jetty HttpClient which supports not only HTTP 2 but
HTTP 1.1 as well
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
