Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-17240: Fix Http2SolrClient maxConnectionsPerDestination usage #2410

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HoustonPutman
Copy link
Contributor

Copy link
Member

@markrmiller markrmiller left a comment

Choose a reason for hiding this comment

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

Oh wow, I thought it was just defaulting to 4 and you couldn't necessarily change that everywhere in internal use because I saw the setMaxConnectionsPerHost in the builder. I didn't realize that that is a complete noop for http2 mode. That seems like a silly choice in any scenario.

@magibney
Copy link
Contributor

javadoc should be updated on HttpSolrClientBuilderBase.withMaxConnectionsPerHost(int), explaining the new difference between how this is handled for http1 vs http2.

I wonder, to approximate similar high-level behavior for http1 vs http2, could the http2 client, under the hood, interpret this setting as ((maxConnectionsPerDest - 1) / 300) + 1, or something? I don't like having that 300 constant hardcoded there, as presumably the multiplexing factor in jetty could change ... but idk, something along those lines might address Sanjay's concern.

@HoustonPutman
Copy link
Contributor Author

Ok I've updated the docs, and used the maxConcurrentPushedStreams to get the multiplexing factor.

This should probably be good to go?

@markrmiller
Copy link
Member

under the hood, interpret this setting as ((maxConnectionsPerDest - 1) / 300) + 1, or something? I don't like having that 300 constant hardcoded there, as presumably the multiplexing factor in jetty could change ... but idk, something along those lines might address Sanjay's concern.

I don't think you want to do that, this should do what it says and set the max connections and not relate to multiplexing. Multiplexing should be it's own setter if you want it be configurable but trying to tie the two together really wouldn't do the user any favors.

@HoustonPutman
Copy link
Contributor Author

under the hood, interpret this setting as ((maxConnectionsPerDest - 1) / 300) + 1, or something? I don't like having that 300 constant hardcoded there, as presumably the multiplexing factor in jetty could change ... but idk, something along those lines might address Sanjay's concern.

I don't think you want to do that, this should do what it says and set the max connections and not relate to multiplexing. Multiplexing should be it's own setter if you want it be configurable but trying to tie the two together really wouldn't do the user any favors.

Fair point mark. I'll make these things independent, add documentation, and make the defaults across solr make sense for both http1 and http2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants