dsmiley commented on code in PR #3852:
URL: https://github.com/apache/solr/pull/3852#discussion_r2506316054
##########
build.gradle:
##########
Review Comment:
oops; will revert
##########
solr/solrj/build.gradle:
##########
@@ -141,14 +148,14 @@ def generatedFiles =
files("${project.generatedCodeDir}/src/main/java") {
/**
* Setup Spotless (Code formatting) for the generated java files
*/
-def generatedExt = new JavaExtension(spotless)
-project.spotlessJavaSetup.execute(generatedExt)
-generatedExt.target(generatedFiles)
-def generatedSpotlessTask =
generatedExt.createIndependentApplyTask("generatedSpotless")
-generatedSpotlessTask.group("build")
-generatedSpotlessTask.description("Apply formatting for generated code")
-
-tasks.openApiGenerate.finalizedBy generatedSpotlessTask
+//def generatedExt = new JavaExtension(spotless)
Review Comment:
oops; this should be a nocommit as I was troubleshooting
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
Review Comment:
javadocs were basically general; should have been placed on the most base
class that they were applicable to. The code examples were for this class but
whatever.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java:
##########
@@ -506,10 +564,8 @@ private NamedList<Object> doRequest(
// Some implementations of LBSolrClient.getClient(...) return a
Http2SolrClient that may not be
// pointed at the desired URL (or any URL for that matter). We special
case that here to ensure
// the appropriate URL is provided.
- if (solrClient instanceof Http2SolrClient httpSolrClient) {
- return httpSolrClient.requestWithBaseUrl(baseUrl, (c) ->
c.request(solrRequest, collection));
- } else if (solrClient instanceof HttpJdkSolrClient) {
- return ((HttpJdkSolrClient) solrClient).requestWithBaseUrl(baseUrl,
solrRequest, collection);
+ if (solrClient instanceof HttpSolrClientBase hasReqWithUrl) {
Review Comment:
note: a TODO still exists on this method referencing a now-closed JIRA...
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -461,5 +458,33 @@ public CloudHttp2SolrClient build() {
return new CloudHttp2SolrClient(this);
}
+
+ protected HttpSolrClientBase createOrGetHttpClient() {
+ if (httpClient != null) {
+ return httpClient;
+ } else if (internalClientBuilder != null) {
+ return internalClientBuilder.build();
+ }
+
+ HttpSolrClientBuilderBase<?, ?> builder;
+ if (HTTP_JETTY_SOLR_CLIENT_BUILDER_CTOR != null) {
+ try {
+ log.debug("Using Http2SolrClient as the delegate http client");
+ builder = HTTP_JETTY_SOLR_CLIENT_BUILDER_CTOR.newInstance();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ } else {
+ log.debug("Using HttpJdkSolrClient as the delegate http client");
+ builder = new HttpJdkSolrClient.Builder();
+ }
+ return builder.build();
+ }
+
+ protected LBSolrClient createOrGetLbClient(HttpSolrClientBase myClient) {
+ return myClient.getLBSolrClient();
Review Comment:
This is why I added getLBSolrClient to the base. This way Jetty & JDK impls
can do their own thing.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java:
##########
@@ -340,6 +347,14 @@ protected void setParser(ResponseParser parser) {
protected abstract void updateDefaultMimeTypeForParser();
+ /** Experimental; subject to change! */
+ @Deprecated // for internal use; expected to change soon
+ public abstract NamedList<Object> requestWithBaseUrlNl(
Review Comment:
I chose to use the same argument order as a *very* similar method in
Http2SolrClient... but then added a suffix to reflect returning a NamedList,
which is the only difference. Marking this as very experimental/internal for
now. This method is only used by LBSolrClient, if I recall.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -553,6 +556,27 @@ protected String
allProcessorSupportedContentTypesCommaDelimited(
return new HttpJdkSolrClient.Builder().withHttpClient(this);
}
+ @Override
+ protected LBSolrClient getLBSolrClient() {
+ return new LBSolrClient(List.of()) {
Review Comment:
Was too simple to even bother naming this thing.
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -102,7 +102,14 @@
* HttpJettySolrClient}.
*/
public class Http2SolrClient extends HttpSolrClientBase {
+ // nocommit rename to HttpJettySolrClient
+
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ /**
+ * A Java system property to select the {@linkplain
HttpClientBuilderFactory} used for configuring
+ * HTTP based SolrClients.
+ */
+ public static final String SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY =
"solr.httpclient.builder.factory"; // nocommit
Review Comment:
Should be renamed, like maybe `solr.solrj.jetty.factory`? (for ref guide as
well).
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -44,20 +44,31 @@ public class CloudHttp2SolrClient extends CloudSolrClient {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private final ClusterStateProvider stateProvider;
- private final LBHttp2SolrClient<HttpSolrClientBase> lbClient;
+ private final LBSolrClient lbClient;
Review Comment:
We can use a plain LBSolrClient!
##########
solr/solrj/build.gradle:
##########
@@ -19,11 +19,9 @@ plugins {
alias(libs.plugins.openapi.generator)
alias(libs.plugins.diffplug.spotless)
id 'java-library'
+ id 'java-test-fixtures'
Review Comment:
Gradle encourages this to share test infra. But I'm doubting our use of it
here was worthwhile so I may use a different strategy.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -82,6 +82,12 @@
import org.slf4j.LoggerFactory;
import org.slf4j.MDC;
+/**
+ * A {@link SolrClient} that routes requests to ideal nodes, including
splitting update batches to
+ * the correct shards. It uses {@link LBSolrClient} as well, thus offering
fail-over abilities if a
+ * core or node becomes unavailable. It's able to know where to route requests
due to its knowledge
+ * of the SolrCloud "cluster state".
+ */
public abstract class CloudSolrClient extends SolrClient {
Review Comment:
it was a crime to have left this undocumented all these years!
##########
solr/solrj/src/test-fixtures/org/apache/solr/client/solrj/impl/ServletFixtures.java:
##########
Review Comment:
Just moved some inner classes of some other test to a place to encourage
sharing. Note that there were *two* DebugServlet impls with slightly similar
implementations! I took the more evolved one.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java:
##########
Review Comment:
TODO: use static imports to reduce the diff
--
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]
