SOLR-17161 Create the solrj-jetty module #4038

janhoy · 2026-01-07T14:31:31Z

Separates out new solr-solrj module and moves the jetty specific classes from solrj.

Needed to generalize ConcurrentUpdateBaseSolrClient, and Claude Code chose to do that by abstracting out a new StreamingResponse class.

I did NOT implement a ConcurrentUpdateJdkSolrClient in this PR, although it should be possible based on the new generic base class...

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

Fix test deps between solrj and solrj-jetty

janhoy · 2026-01-07T15:06:32Z

This is the dependency:tree for org.apache.solr:solr-solrj before this change:

org.example:testsolrj:jar:1.0-SNAPSHOT
\- org.apache.solr:solr-solrj:jar:11.0.0-SNAPSHOT:compile
   +- org.apache.solr:solr-api:jar:11.0.0-SNAPSHOT:compile
   |  +- io.swagger.core.v3:swagger-annotations-jakarta:jar:2.2.22:compile
   |  +- jakarta.ws.rs:jakarta.ws.rs-api:jar:3.1.0:runtime
   |  \- org.semver4j:semver4j:jar:6.0.0:runtime
   |     \- org.jspecify:jspecify:jar:1.0.0:runtime
   +- org.eclipse.jetty.http2:jetty-http2-client:jar:12.0.27:compile
   |  +- org.eclipse.jetty:jetty-alpn-client:jar:12.0.27:compile
   |  \- org.eclipse.jetty.http2:jetty-http2-common:jar:12.0.27:compile
   |     \- org.eclipse.jetty.http2:jetty-http2-hpack:jar:12.0.27:compile
   +- com.fasterxml.jackson.core:jackson-databind:jar:2.20.1:runtime
   +- com.fasterxml.jackson.core:jackson-annotations:jar:2.20:compile
   +- com.fasterxml.jackson.core:jackson-core:jar:2.20.1:runtime
   +- org.slf4j:slf4j-api:jar:2.0.17:compile
   +- org.eclipse.jetty.http2:jetty-http2-client-transport:jar:12.0.27:runtime
   +- org.eclipse.jetty:jetty-http:jar:12.0.27:compile
   +- org.eclipse.jetty:jetty-client:jar:12.0.27:runtime
   +- org.eclipse.jetty:jetty-util:jar:12.0.27:compile
   +- org.eclipse.jetty:jetty-io:jar:12.0.27:compile
   +- org.slf4j:jcl-over-slf4j:jar:2.0.17:runtime
   \- org.eclipse.jetty:jetty-alpn-java-client:jar:12.0.27:runtime

And this is after

org.example:testsolrj:jar:1.0-SNAPSHOT
\- org.apache.solr:solr-solrj:jar:11.0.0-SNAPSHOT:compile
   +- org.apache.solr:solr-api:jar:11.0.0-SNAPSHOT:compile
   |  +- io.swagger.core.v3:swagger-annotations-jakarta:jar:2.2.22:compile
   |  +- jakarta.ws.rs:jakarta.ws.rs-api:jar:3.1.0:runtime
   |  \- org.semver4j:semver4j:jar:6.0.0:runtime
   |     \- org.jspecify:jspecify:jar:1.0.0:runtime
   +- com.fasterxml.jackson.core:jackson-databind:jar:2.20.1:runtime
   +- com.fasterxml.jackson.core:jackson-annotations:jar:2.20:compile
   +- com.fasterxml.jackson.core:jackson-core:jar:2.20.1:runtime
   +- org.slf4j:slf4j-api:jar:2.0.17:runtime
   \- org.slf4j:jcl-over-slf4j:jar:2.0.17:runtime

janhoy · 2026-01-07T17:28:18Z

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

   * @param respBody the body of the response, subclasses must not close this stream.
   */
-  public void onSuccess(Response resp, InputStream respBody) {
+  public void onSuccess(Object responseMetadata, InputStream respBody) {


I dont like Object as type, can we imagine a more specific type that is generic enough for the purpose?

@dsmiley Object is simple, I guess a modern alternative is to use generics:

public class ConcurrentUpdateJettySolrClient extends ConcurrentUpdateBaseSolrClient<org.eclipse.jetty.client.Response>

and

public interface StreamingResponse<R>

Touches all the same 5 files and all signatures, avoids Object but the API perhaps gets more complicated to understand..

Here is a PR against this feature-branch for how it will look when using generics janhoy#15 @jdyer1 what is your preference since you'll be implementing a sub class for JDK client?

IMO we shouldn't add generics for an internal matter -- which is what this is (?). Meaning a user of the client shouldn't have to be confronted with it, in general.

The generics part only helps type things in the sub classes. The end user will relate to ConcurrentUpdateJettySolrClient, not the generalized base classes...

I see. Up to you; I don't care.

dsmiley

Thanks for getting this across the line! If I had known 10.0 RC would be in 2026, I'd have done it.

Just some minor feedback. The use of "Object" where you commented is fine.

...solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/ConcurrentUpdateJettySolrClient.java

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

dsmiley · 2026-01-08T05:45:26Z

solr/solrj-jetty/build.gradle

+  })
+
+  // Permit unused test dependencies (transitive dependencies)
+  permitTestUnusedDeclared libs.commonsio.commonsio


Did you actually have to add all of these to make the build happy?
Use of them is each a work-around to a deficiency in the cutterslade plugin . Sometimes I wonder if we'd be better off without that thing. Maybe sharing this pittiful situation here with that project may motivate a proper fix there.

Yea, without these, we get:

FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':solr:solrj-jetty:analyzeTestClassesDependencies'. > Dependency analysis found issues. unusedDeclaredArtifacts - commons-io:commons-io:2.20.0@jar - org.apache.httpcomponents:httpclient:4.5.14@jar - org.apache.httpcomponents:httpcore:4.4.16@jar - org.apache.lucene:lucene-core:10.3.2@jar - org.apache.zookeeper:zookeeper-jute:3.9.4@jar - org.eclipse.jetty.ee10:jetty-ee10-webapp:12.0.27@jar - org.eclipse.jetty:jetty-server:12.0.27@jar - org.eclipse.jetty:jetty-session:12.0.27@jar - org.mockito:mockito-core:5.19.0@jar

Not sure if there is a better solution? @malliaridis ?

Christos didn't intorduce this plugin; ask @risdenk who is a fan of it. I have mixed feelings due to its faults.

I think this is correct... why do we have

commonsio declared?

testImplementation libs.commonsio.commonsio

do we need any of the ones listed?

These are saying that these dependencies are declared but unused.

Like take each one and see if we can just remove them

testImplementation libs.apache.httpcomponents.httpclient testImplementation libs.apache.httpcomponents.httpcore

I would guess we don't need them. There are very few exceptions needed if any.

All the check is saying is that as defined, the dependencies are wrong. they don't need to be testImplementation. Implementation implies that they need to be here for compilation of this specific module.

Maybe they need to be test runtime? I don't know. but I would assume that should happen through references to any other modules.

From what I can see the check is correct and we don't need to define these dependencies this way. If they are needed at runtime thats what testRuntime is for and would also satisfy the check without needing to purposefully override it.

Apache httpclient and our SolrClient implementations that use it should come transitively via our solr-test-framework. Unless this module is explicitly (directly) referencing Apache httpclient classes, we need not reference it. Transitive doesn't count.

Did you put these references here guessing they might be needed? Please don't do that if so. Let the build (pass/fail) guide you on what we need to add.

Can you review the other test dependencies?

This is for you to do as the author of this new gradle module. It takes no expertise/knowledge... start from ~nothing and iteratively add what you need. The build will inform you of what you need :-) Don't guess what you might need. This cutterslade plugin is designed to stop you from guessing wrong :-)

It was some back and forth with how to wire this in the process and with AI, so it's not fully built stone on stone, more zig-zag. I'll see if testRuntime can be used.

I'll see if testRuntime can be used.

I think you don't need such a statement which is even better. Use testRuntime if there's a dependency needed at runtime (but not compile) that gradle doesn't know about (meaning it doesn't come transitively). An example would be an optional dependency where we do use the feature that activates that optionality. Like maybe using SolrJ and we know we want SolrJ-Zookeeper. That doesn't apply to this case.

solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java

Move private class lower

janhoy · 2026-01-09T18:31:11Z

I'm ok with having this in all in one PR, but if folks feel strongly about it we could split out the refactoring of ConcurrentUpdateBaseSolrClient as an independent preparatory PR..

janhoy added 3 commits January 7, 2026 13:32

SOLR-17161 Raw refactoring by Claude Code

322f3ce

Add solrj-jetty as dep where needed

fbe16ad

Fix test deps between solrj and solrj-jetty

Fix onSuccess overrides

f5d3c08

More specific exception on close()

cdf2985

janhoy marked this pull request as draft January 7, 2026 14:44

Changelog

c8fb99f

janhoy requested a review from dsmiley January 7, 2026 14:48

janhoy commented Jan 7, 2026

View reviewed changes

janhoy requested a review from Copilot January 7, 2026 17:29

Copilot started reviewing on behalf of janhoy January 7, 2026 17:29 View session

This comment was marked as resolved.

Sign in to view

Fix refguide links

72ec8a6

dsmiley approved these changes Jan 8, 2026

View reviewed changes

janhoy added 2 commits January 8, 2026 12:01

Make interface inner

04011a8

Move private class lower

Rename variable

a635b77

janhoy marked this pull request as ready for review January 8, 2026 11:11

janhoy requested a review from jdyer1 January 9, 2026 18:23

Merge branch 'main' into SOLR-17161-separate-jetty-solrj

c918e14

Add info about ConcurrentUpdateBaseSolrClient to changelog entry

dbe598c

SOLR-17161 Create the solrj-jetty module #4038

Are you sure you want to change the base?

SOLR-17161 Create the solrj-jetty module #4038

Conversation

janhoy commented Jan 7, 2026

Uh oh!

janhoy commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

This comment was marked as resolved.

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janhoy commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

janhoy commented Jan 7, 2026 •

edited

Loading

janhoy commented Jan 9, 2026 •

edited

Loading