gerlowskija commented on code in PR #3852:
URL: https://github.com/apache/solr/pull/3852#discussion_r2531930287
##########
solr/benchmark/build.gradle:
##########
@@ -44,6 +44,7 @@ dependencies {
implementation project(':solr:test-framework')
implementation project(':solr:core')
implementation project(':solr:solrj')
+ implementation project(':solr:solrj-jetty')
Review Comment:
[Q] There's been [some discussion in the
past](https://issues.apache.org/jira/browse/SOLR-17161?focusedCommentId=17892594&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17892594)
that this new solrj-jetty artifact would be "opt-out", meaning that folks
would get it implicitly by depending on `:solr:solrj`
Is that no longer the plan? Or if it is the plan, why do we need the
explicit dependency on solrj-jetty here and in other similar build.gradle files?
##########
solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc:
##########
@@ -72,6 +72,10 @@ That no longer happens in Solr 10. It is only used to load
a configuration as a
* Starting in 10, the Maven POM for SolrJ does not refer to SolrJ modules like
ZooKeeper. If you require such functionality, you need to add additional
dependencies.
+* Classes using Jetty HttpClient have been extracted to a new module
"solrj-jetty" and moved to a new package `org.apache.solr.solrj.jetty`, and
possibly renamed.
+Renames: `Http2SolrClient` to `HttpJettySolrClient`,
`ConcurrentUpdateHttp2SolrClient` to `ConcurrentUpdateJettySolrClient`,
`LBHttp2SolrClient` to `LBAsyncSolrClient`, adding LBJettySolrClient.
Review Comment:
[-0] I'm not sure whether this renaming is planned and just hasn't happened
yet, but it's not currently accurate. Ignore this comment if that's intended
and just a matter of WIP.
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/LBJettySolrClient.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj.jetty;
+
+import java.io.IOException;
+import java.util.concurrent.CompletableFuture;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+import org.apache.solr.client.solrj.impl.LBHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.LBSolrClient;
+import org.apache.solr.common.util.NamedList;
+
+/** An {@link LBSolrClient} based on Jetty HttpClient, supporting async. */
+public class LBJettySolrClient extends LBHttp2SolrClient<Http2SolrClient> {
+
+ protected LBJettySolrClient(Builder builder) {
+ super(builder);
+ }
+
+ public static class Builder extends LBSolrClient.Builder<Http2SolrClient> {
+
+ public Builder(Http2SolrClient solrClient, Endpoint... endpoints) {
+ super(solrClient, endpoints);
+ }
+
+ @Override
+ public LBJettySolrClient build() {
+ return new LBJettySolrClient(this);
+ }
+ }
+
+ @Override
+ protected CompletableFuture<NamedList<Object>> requestAsyncWithUrl(
Review Comment:
[0] Small nit - I often find these methods get missed if they come after
static class definitions. Might be worth moving this above the Builder if you
agree.
##########
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 think I agree with James. I really want to see the non-NL return types
when they're eventually ready, but I don't think we should muddy the signatures
in advance of that. We can cross that bridge when we come to it IMO.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/HttpClientBuilderFactory.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj;
+
+/**
+ * A config hook for post-configuration of a {@linkplain SolrClient} by its
builder. It not
+ * supported by all builders.
+ *
+ * @see #CLIENT_CUSTOMIZER_SYSPROP
+ * @lucene.experimental
+ */
+public interface HttpClientBuilderFactory {
+ // nocommit rename to SolrClientCustomizer
+ /**
+ * A Java system property to select the {@linkplain
HttpClientBuilderFactory} used for configuring
+ * HTTP based SolrClients.
+ */
+ String CLIENT_CUSTOMIZER_SYSPROP = "solr.solrj.http.customizer";
+
+ void setup(SolrClient client);
Review Comment:
[-0] I think I understand why the module-creation requires us to widen the
parameter type here from Http2SolrClient to SolrClient, but it's a real bummer
IMO.
It leaves users with no easy way to tell whether a given
ClientBuilderFactory actually supports the client they're trying to use. The
interface implies it works with all SolrClients, why would a user think to look
at the code for (e.g.) PreemptiveAuthCBF and realize that's not at all the case.
Is there anything fancy we could do with type parameters on the interface
that'd allow implementations to indicate the supported types, e.g. `MyImpl
implements HttpClientBuilderFactory<Http2SolrClient>`
Absent that, IMO we should have this method throw an
IllegalArgumentException or indicate incompatibility in some other way...
##########
changelog/unreleased/SOLR-17161-solrj-jetty.yml:
##########
@@ -0,0 +1,9 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: 'SolrJ: separated Jetty HttpClient using classes to a new solrj-jetty
module,
Review Comment:
[0] This changelog entry highlights how much this PR changes from a user
perspective: creating a whole new module, establishing a new Java package,
renaming a bunch of classes that are pretty close to the user, etc.
Maybe that's just "initial impression", and the PR all ends up being pretty
cohesive as I go through it. But it makes me wonder whether it's worth
splitting into multiple PRs. (I guess the obvious place to draw that line
would be on the renames, but maybe there are other fault lines)
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/CloudJettySolrClient.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.client.solrj.jetty;
+
+import java.util.List;
+import org.apache.solr.client.solrj.impl.CloudHttp2SolrClient;
+import org.apache.solr.client.solrj.impl.ClusterStateProvider;
+import org.apache.solr.client.solrj.impl.Http2SolrClient;
+
+/**
+ * A {@link org.apache.solr.client.solrj.impl.CloudSolrClient} using Jetty
{@code HttpClient} for
+ * HTTP communication. This is Solr's most robust CloudSolrClient.
Review Comment:
[Q] "most robust"? What does it offer over "CloudHttp2SolrClient"?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/HttpClientBuilderFactory.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj;
+
+/**
+ * A config hook for post-configuration of a {@linkplain SolrClient} by its
builder. It not
Review Comment:
[0] Typo: "It not" -> "It's not"
##########
solr/solr-ref-guide/modules/deployment-guide/pages/solrj.adoc:
##########
@@ -95,11 +95,11 @@ Requests are sent in the form of
{solr-javadocs}/solrj/org/apache/solr/client/so
`SolrClient` has a few concrete implementations, each geared towards a
different usage-pattern or resiliency model:
--
{solr-javadocs}/solrj/org/apache/solr/client/solrj/impl/Http2SolrClient.html[`Http2SolrClient`]
- a general purpose client based on Jetty HttpClient. Supports HTTP/2 and
HTTP/1.1, async, non-blocking. Most used & tested.
+-
{solr-jetty-javadocs}/solrj/org/apache/solr/client/solrj/impl/Http2SolrClient.html[`Http2SolrClient`]
- a general purpose client based on Jetty HttpClient. Supports HTTP/2 and
HTTP/1.1, async, non-blocking. Most used & tested.
Review Comment:
[0] I'm still a little confused about whether the new "jetty" module is
opt-in or not. But if it's opt-in, maybe we should lead with the "JDK" clients
and have the Jetty clients in a separate list (or at the end of this list).
[-0] If we're adding a new CloudJettySolrClient, it should probably be
mentioned here, right?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java:
##########
@@ -340,6 +347,12 @@ protected void setParser(ResponseParser parser) {
protected abstract void updateDefaultMimeTypeForParser();
+ /** Experimental; subject to change! */
Review Comment:
[Q] Why is this deprecated the moment it's being added?
[0] If we want this to be experimental, can we use the
`@lucene.experimental` annotation to track that as well?
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/impl/PreemptiveBasicAuthClientBuilderFactory.java:
##########
@@ -66,15 +69,16 @@ public static void setDefaultSolrParams(SolrParams params) {
}
@Override
- public void close() throws IOException {}
-
- @Override
- public void setup(Http2SolrClient client) {
+ public void setup(SolrClient client) {
+ if (client instanceof Http2SolrClient == false) {
+ return;
Review Comment:
[-1] Perhaps I'm misreading this, but it seems trappy to return quietly when
the user passes in a client that we don't support configuring. Shouldn't we
throw an exception or something here instead?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -29,13 +29,13 @@
import org.apache.solr.client.solrj.request.RequestWriter;
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.IOUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * SolrJ client class to communicate with SolrCloud using an Http/2-capable
Solr Client. Instances
- * of this class communicate with Zookeeper to discover Solr endpoints for
SolrCloud collections,
- * and then use the {@link LBHttp2SolrClient} to issue requests.
+ * This {@link CloudSolrClient} is a base implementation using a {@link
HttpSolrClientBase}. The '2'
+ * in the name has no differentiating significance anymore.
Review Comment:
[0] Typo: "anymore" -> "any more"
[0] Might be a good place to point users at the setting that allows toggling
of HTTP1/HTTP2, rather than just pointing out that the client supports both.
##########
solr/solrj-jetty/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -626,6 +630,13 @@ public final <R extends SolrResponse> R requestWithBaseUrl(
return requestWithBaseUrl(baseUrl, (c) -> req.process(c, collection));
}
+ @Override
+ public NamedList<Object> requestWithBaseUrlNl(
Review Comment:
[-0/Q] Why this new method that exposes 'Nl' in the name? Do we really need
this?
##########
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:
Yeah, this is awesome!
--
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]
