SOLR-17161: solrj-jetty module #3852

dsmiley · 2025-11-08T06:20:54Z

First iteration. Didn't do renames / package moves yet -- left nocommit for them. Just moved pertinent source files. I'd like a code review now before doing the wrote boring renames that will obscure the substance of the changes.

https://issues.apache.org/jira/browse/SOLR-17161

Tests pass. There seems to be some weird build issue regarding spotless in solrj... perhaps my use of java-test-fixtures plugin confused it. I'm very tempted to just undo the use of that plugin, even if it means basically all testing done from solrj not solrj-jetty. Which is much the case anyway.

First iteration. Didn't do renames yet.

dsmiley · 2025-11-08T06:21:25Z

build.gradle

oops; will revert

dsmiley · 2025-11-08T06:23:40Z

solr/solrj-jetty/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java

+   * 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


Should be renamed, like maybe solr.solrj.jetty.factory? (for ref guide as well).

dsmiley · 2025-11-08T06:25:04Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java

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.

dsmiley · 2025-11-08T06:27:46Z

solr/solrj/build.gradle

  alias(libs.plugins.openapi.generator)
  alias(libs.plugins.diffplug.spotless)
  id 'java-library'
+  id 'java-test-fixtures'


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.

dsmiley · 2025-11-08T06:28:28Z

solr/solrj/build.gradle

-generatedSpotlessTask.description("Apply formatting for generated code")
-
-tasks.openApiGenerate.finalizedBy generatedSpotlessTask
+//def generatedExt = new JavaExtension(spotless)


oops; this should be a nocommit as I was troubleshooting

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java

dsmiley · 2025-11-08T06:35:10Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java


+  /** Experimental; subject to change! */
+  @Deprecated // for internal use; expected to change soon
+  public abstract NamedList<Object> requestWithBaseUrlNl(


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.

In HttpJdkSolrClient all 3 methods, request, requestAsync and requestWithBaseUrl take a request and collection and return a NamedList. This seems like a sensible api. Yet Http2SolrClient has requestWithBaseUrl with a different signature. I am not sure what your intention here is, but it seems to me the api used by the jdk client would make the most sense to users. (Side point here, but the current behavior of Http2SolClient#requestWthBaseUrl where it basically clones itself on each request seems rather unfortunate.)

I hear you... but we're also (slowly) working towards return types that do not presume a NamedList. CC @gerlowskija curious on your thoughts here.

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.

dsmiley · 2025-11-08T06:36:17Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java

-      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) {


note: a TODO still exists on this method referencing a now-closed JIRA...

dsmiley · 2025-11-08T06:37:33Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/ServletFixtures.java

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.

dsmiley · 2025-11-08T06:38:44Z

solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java

TODO: use static imports to reduce the diff

…syncSolrClient. Moved its builder to LBSolrClient. Added LBJettySolrClient. LBHttp2SolrClient is now abstract with one impl.

dsmiley · 2025-11-10T03:09:11Z

This is in a reviewable state. The renames are nocommit TODOs that I'm intentionally deferring until after preliminary review. See JIRA comments for high level points. The commits are relatively self contained.

dsmiley · 2025-11-13T13:45:43Z

It's been 5 days. On Monday I'm going to proceed with the renames I proposed. I suppose this weekend (or sooner if asked), I'll may squash this PR into ~1-3 commits to help reviewers that come late to look at these instead of the 17 now. Maybe I'll separate the noisy lockfile changes. The renames on Monday will be mechanical, done by IntelliJ. If there's interesting/non-obvious parts, I will comment in a self-review.

epugh · 2025-11-13T14:14:46Z

Sounds like a plan!

gerlowskija

Left some questions in review. Mostly tried to steer clear of any of the naming decisions, as I already left a comment about those in JIRA for SOLR-17161.

One minor process nit - this PR is doing a lot of different things: we're renaming a handful of our user-facing clients, we're creating a new Gradle module, we're adding async methods to a new LB client, we're creating a new "Cloud" client targeting Jetty specifically, we're refactoring some of our Servlet test logic into a utility class, etc.

I know this stuff kindof grows organically and sometimes the possible break-points in that growth are only visible in retrospect. But it would be much easier to discuss and review if this was 3 or 4 different JIRAs+PRs, rather than one big one.

gerlowskija · 2025-11-16T12:37:23Z

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,


[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)

I'm splitting.

gerlowskija · 2025-11-16T12:41:09Z

solr/benchmark/build.gradle

  implementation project(':solr:test-framework')
  implementation project(':solr:core')
  implementation project(':solr:solrj')
+  implementation project(':solr:solrj-jetty')


[Q] There's been some discussion in the past 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?

No change in plan.
We need this explicit dependency because solrj-jetty isn't an api dependency of solrj, it's implementation, which makes sense to me. So if a module (like benchmark) wants to refer to specific classes in this module, it needs to. Ideally it wouldn't, ideally runtime would be enough.

gerlowskija · 2025-11-17T11:54:02Z

solr/solr-ref-guide/modules/deployment-guide/pages/solrj.adoc

 `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.


[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?

Since it is opt-out, I we agree on the ordering.
Yes, CloudJettySolrClient should be listed -- good catch.

gerlowskija · 2025-11-17T11:56:00Z

solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc

 * 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.


[-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.

These are nocommits; I was going to do them today but instead I'm going to spend time splitting up the work and defer the bigger renames.

solr/solrj/src/java/org/apache/solr/client/solrj/HttpClientBuilderFactory.java

gerlowskija · 2025-11-17T12:36:53Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java

- * 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.


[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.

Thanks. I don't want to link to details; this is more of a feature-list. Users should consult the builder (of whatever client) for detailed configuration.

gerlowskija · 2025-11-17T12:40:52Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java

+ * 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 {


Yeah, this is awesome!

gerlowskija · 2025-11-17T12:44:38Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java


  protected abstract void updateDefaultMimeTypeForParser();

+  /** Experimental; subject to change! */


[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?

I considered it so subject to change that I wanted a bit extra warning :-). Anyway, we agreed on a new signature that I will apply soon.

gerlowskija · 2025-11-17T12:58:21Z

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java


+  /** Experimental; subject to change! */
+  @Deprecated // for internal use; expected to change soon
+  public abstract NamedList<Object> requestWithBaseUrlNl(


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.

gerlowskija · 2025-11-17T13:51:15Z

...etty/src/java/org/apache/solr/client/solrj/impl/PreemptiveBasicAuthClientBuilderFactory.java

-  public void setup(Http2SolrClient client) {
+  public void setup(SolrClient client) {
+    if (client instanceof Http2SolrClient == false) {
+      return;


[-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?

I'll respond to this in the new PR I created in a couple minutes

dsmiley · 2025-11-17T14:35:32Z

Thanks @gerlowskija! I needed to push on this to understand what all the different matters are. Having done so, I will break this up into a number of separate PRs. I suppose JIRA issues as well; maybe sub-tasks where JIRA-worthy.

dsmiley · 2025-11-20T03:22:41Z

I created a number of follow-up PRs to do this piecemeal. This PR is paused until most of those complete.

SOLR-17161: solrj-jetty module

6a5b484

First iteration. Didn't do renames yet.

dsmiley requested a review from jdyer1 November 8, 2025 06:20

github-actions bot added dependencies Dependency upgrades test-framework client:solrj module:extraction module:jwt-auth module:ltr module:opentelemetry module:s3-repository tool:build module:gcs-repository module:sql tests cat:cloud module:clustering module:langid module:scripting labels Nov 8, 2025

dsmiley commented Nov 8, 2025

View reviewed changes

dsmiley added 9 commits November 8, 2025 10:34

address self-review.

88cd630

changelog & ref guide

049b61e

HttpClientBuilderFactory / Customizer

f3c2b4f

package-infos

ad9978d

javadoc: no inheritDoc on class

f018030

fix test compile

2889731

Undo fixtures

ef5d878

test: use static imports for ServletFixtures

ccbf35f

tidy

e389b32

github-actions bot added the documentation Improvements or additions to documentation label Nov 8, 2025

ref guide links

bdda63c

dsmiley added 5 commits November 8, 2025 21:12

build.gradle references

f2277f6

gradle.lockfile

e9a0012

Close CloudSolrClients more robustly (null safe, from mocks)

b4415cf

rename getLBSolrClient to createLBSolrClient

945b231

fix DebugServlet header lowerCase

2117b5a

github-actions bot added admin-ui cat:api labels Nov 9, 2025

LBHttp2SolrClient returns to solrj core. Later, suggest rename to LBA…

7c70be8

…syncSolrClient. Moved its builder to LBSolrClient. Added LBJettySolrClient. LBHttp2SolrClient is now abstract with one impl.

github-actions bot added the cat:search label Nov 10, 2025

dsmiley requested a review from gerlowskija November 10, 2025 03:08

dsmiley requested a review from janhoy November 13, 2025 13:46

gerlowskija reviewed Nov 17, 2025

View reviewed changes

		@@ -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,


		protected abstract void updateDefaultMimeTypeForParser();

		/** Experimental; subject to change! */

SOLR-17161: solrj-jetty module #3852

Are you sure you want to change the base?

SOLR-17161: solrj-jetty module #3852

Conversation

dsmiley commented Nov 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Nov 10, 2025

Uh oh!

dsmiley commented Nov 13, 2025

Uh oh!

epugh commented Nov 13, 2025

Uh oh!

gerlowskija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Nov 17, 2025

Uh oh!

dsmiley commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels