Skip to content

Conversation

@dsmiley
Copy link
Contributor

@dsmiley dsmiley commented Nov 29, 2025

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

Moving jetty client stuff to the jetty package, that haven't yet moved there.
Renamed Http2SolrClient to HttpJettySolrClient.
CloudJettySolrClient added, and enhanced CloudHttp2SolrClient to detect if Jetty is available and to use that client if it is -- (same code here as provided in #3852)

Move CloudHttp2SolrClient.Builder to CloudSolrClient. There's now barely any references to CloudHttp2SolrClient. We'll probably remove that someday. I marked it as internal.

This'll be tough to review due to rename noise... I think it's boring honestly but the changes in the impl & jetty packages could be reviewed.

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I scanned, and while I don't love the use of var, that is just me. The doc changes looked good. And the overall change looks good.

Copy link
Contributor Author

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added var systematically for HttpJettySolrClient (or via its Builder) for the case of calling a constructor to initialize it. So it's type is obvious, in theory, albeit I didn't look at every single line. Do you find the type unclear anywhere @epugh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @jdyer1 ... even though HttpClusterStateProvider is exists in the apache package in the test framework, it will be removed, freeing up this name for use here.

*/
public abstract ConcurrentUpdateBaseSolrClient build();

public HttpSolrClientBase getClient() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed occasional getters to expose things that are now in different packages and thus not exposed. A protected field on a builder isn't accessible to the class being constructed that uses the builder, if it's in another package.

@epugh
Copy link
Contributor

epugh commented Nov 29, 2025

I added var systematically for HttpJettySolrClient (or via its Builder) for the case of calling a constructor to initialize it. So it's type is obvious, in theory, albeit I didn't look at every single line. Do you find the type unclear anywhere @epugh

I think for me it's just a stylistic thing that twenty years of Java typing has leaned me against! In ruby it is all vars ;-)

@dsmiley
Copy link
Contributor Author

dsmiley commented Nov 29, 2025

I'm planning on merging this one late tonight or tomorrow morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants