dsmiley commented on code in PR #2259:
URL: https://github.com/apache/solr/pull/2259#discussion_r1498447578
##########
solr/solrj/src/java/org/apache/solr/client/solrj/embedded/SSLConfigBase.java:
##########
@@ -40,35 +52,75 @@ public SSLConfigBase(
this.trustStorePassword = trustStorePassword;
}
+ /**
+ * If set to false, all other settings are ignored.
+ *
+ * @param useSsl boolean
+ */
public void setUseSSL(boolean useSsl) {
this.useSsl = useSsl;
}
+ /**
+ * Set whether client authentication should be requested. If set to false,
truststore settings are
+ * ignored.
+ *
+ * @param clientAuth boolean
+ */
public void setClientAuth(boolean clientAuth) {
this.clientAuth = clientAuth;
}
- /** All other settings on this object are ignored unless this is true */
+ /**
+ * All other settings on this object are ignored unless this is true
+ *
+ * @return boolean
+ */
public boolean isSSLMode() {
return useSsl;
}
+ /**
+ * Whether client authentication should be requested. If false, truststore
settings are ignored.
+ *
+ * @return boolean
+ */
public boolean isClientAuthMode() {
return clientAuth;
}
+ /**
+ * The keystore to use.
Review Comment:
I don't know much about keystores; do they have names or is this a path?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -239,16 +244,16 @@ private void setBasicAuthHeader(SolrRequest<?>
solrRequest, HttpRequest.Builder
}
}
- private static final Pattern MIME_TYPE_PATTERN =
Pattern.compile("^(.*?)(?:;| |$)");
+ private static final Pattern MIME_TYPE_PATTERN = Pattern.compile("[^;]*");
private String contentTypeToMimeType(String contentType) {
Matcher mimeTypeMatcher = MIME_TYPE_PATTERN.matcher(contentType);
- return mimeTypeMatcher.find() ? mimeTypeMatcher.group(1) : null;
+ return mimeTypeMatcher.find() ? mimeTypeMatcher.group() : null;
}
- private static final Pattern CHARSET_PATTERN =
Pattern.compile("(?i)^.*charset=(.*)?(?:;| |$)");
+ private static final Pattern CHARSET_PATTERN =
Pattern.compile("(?i)^.*charset=(.*)$");
Review Comment:
I was thinking maybe `(?i)[; ]charset=([^;]*)` but not important
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -24,7 +24,8 @@
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.client.solrj.request.RequestWriter;
-public abstract class HttpSolrClientBuilderBase {
+public abstract class HttpSolrClientBuilderBase<
+ B extends HttpSolrClientBuilderBase<?, ?>, C extends Http2SolrClientBase> {
Review Comment:
Is it overly specific to insist that C extends Http2SolrClientBase? In
other words, can just say it's a SolrClient? The "2" in there makes me hope we
can remove it one way or another.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -36,34 +36,32 @@
import org.eclipse.jetty.util.ssl.SslContextFactory;
import org.junit.Test;
-public class Http2SolrClientTest extends
Http2SolrClientTestBase<Http2SolrClient.Builder> {
+public class Http2SolrClientTest extends Http2SolrClientTestBase {
@Override
protected String expectedUserAgent() {
return "Solr[" + Http2SolrClient.class.getName() + "] 2.0";
}
@Override
- protected <B extends HttpSolrClientBuilderBase> B builder(
- String url, int connectionTimeout, int socketTimeout, Class<B> type) {
Review Comment:
I'm on the fence of you removing `Class<B> type` for the builder. I *think*
this would enable you to remove all those SuppressWarning lines.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java:
##########
@@ -43,43 +44,43 @@ public abstract class HttpSolrClientBuilderBase {
protected boolean proxyIsSocks4;
protected boolean proxyIsSecure;
- public HttpSolrClientBuilderBase() {}
-
- protected abstract <B extends Http2SolrClientBase> B build(Class<B> type);
-
- public HttpSolrClientBuilderBase(String baseSolrUrl) {
- this.baseSolrUrl = baseSolrUrl;
- }
+ public abstract C build();
/** Provides a {@link RequestWriter} for created clients to use when handing
requests. */
- public HttpSolrClientBuilderBase withRequestWriter(RequestWriter
requestWriter) {
+ @SuppressWarnings(value = "unchecked")
+ public B withRequestWriter(RequestWriter requestWriter) {
this.requestWriter = requestWriter;
- return this;
+ return (B) this;
}
/** Provides a {@link ResponseParser} for created clients to use when
handling requests. */
- public HttpSolrClientBuilderBase withResponseParser(ResponseParser
responseParser) {
+ @SuppressWarnings(value = "unchecked")
+ public B withResponseParser(ResponseParser responseParser) {
this.responseParser = responseParser;
- return this;
+ return (B) this;
}
/** Sets a default for core or collection based requests. */
- public HttpSolrClientBuilderBase withDefaultCollection(String
defaultCoreOrCollection) {
+ @SuppressWarnings(value = "unchecked")
+ public B withDefaultCollection(String defaultCoreOrCollection) {
this.defaultCollection = defaultCoreOrCollection;
- return this;
+ return (B) this;
}
- public HttpSolrClientBuilderBase withFollowRedirects(boolean
followRedirects) {
+ @SuppressWarnings(value = "unchecked")
Review Comment:
The `value = ` part isn't necessary as it's the default param
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/DebugServlet.java:
##########
@@ -125,6 +126,11 @@ private void recordRequest(HttpServletRequest req,
HttpServletResponse resp) {
setParameters(req);
setQueryString(req);
setCookies(req);
+ try {
+ requestBody = req.getInputStream().readAllBytes();
Review Comment:
some tidy formatting to do, I see
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -335,7 +327,7 @@ public void testCollectionParameters() throws IOException,
SolrServerException {
@Test
public void testQueryString() throws Exception {
- testQueryString(Http2SolrClient.class, Http2SolrClient.Builder.class);
+ super.testQueryString();
}
Review Comment:
I *think* this isn't needed, as long as testQueryString in the superclass is
marked as a test like this one.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -75,9 +77,16 @@ protected HttpSolrClientJdkImpl(String serverBaseUrl,
HttpSolrClientJdkImpl.Buil
if (builder.sslContext != null) {
b.sslContext(builder.sslContext);
}
+
if (builder.executor != null) {
- b.executor(builder.executor);
+ this.executor = builder.executor;
+ this.shutdownExecutor = false;
+ } else {
+ this.executor =
Executors.newCachedThreadPool(Executors.defaultThreadFactory());
Review Comment:
Normally we use `ExecutorUtil`. Are you copying existing code? FYI
@iamsanjay is working on a PR that will likely touch this because the executor
initialization is a bit inconsistent / broken.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/embedded/SSLConfigBase.java:
##########
@@ -40,35 +52,75 @@ public SSLConfigBase(
this.trustStorePassword = trustStorePassword;
}
+ /**
+ * If set to false, all other settings are ignored.
+ *
+ * @param useSsl boolean
+ */
public void setUseSSL(boolean useSsl) {
this.useSsl = useSsl;
}
+ /**
+ * Set whether client authentication should be requested. If set to false,
truststore settings are
+ * ignored.
+ *
+ * @param clientAuth boolean
+ */
public void setClientAuth(boolean clientAuth) {
this.clientAuth = clientAuth;
}
- /** All other settings on this object are ignored unless this is true */
+ /**
+ * All other settings on this object are ignored unless this is true
+ *
+ * @return boolean
Review Comment:
You should remove these useless lines. In the end your javadoc will be so
short that tidy will put it on one actual line, which is rather satisfying.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClientBase.java:
##########
@@ -371,6 +371,10 @@ public boolean isV2ApiRequest(final SolrRequest<?>
request) {
return request instanceof V2Request ||
request.getPath().contains("/____v2");
}
+ public String getBaseURL() {
Review Comment:
Our current project naming conventions are to use "Url" capitalization in
this case.
--
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
