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

Configuration for setting maximum number of connection pools to be created #3566

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

Conversation

jchenga
Copy link
Contributor

@jchenga jchenga commented Dec 29, 2024

Introduce a configuration option to specify the maximum number of connection pools a ConnectionProvider should create. Once the maximum number is exceeded, a warning message is logged.

Fixes #3318.

…nnection pools a ConnectionProvider should create. Once the expected number is exceeded, a warning message is logged.
@violetagg violetagg added the type/enhancement A general enhancement label Jan 2, 2025
@violetagg violetagg added this to the 1.2.2 milestone Jan 2, 2025
@violetagg violetagg changed the title Configuration for setting expected number of connection pools to be created Configuration for setting maximum number of connection pools to be created Jan 2, 2025
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

@jchenga Thanks for the PR!
I do have some suggestions for the name of the new API and for the tests.

@jchenga jchenga force-pushed the 3318-configuration-for-connection-pools-limit branch from f64e1cd to e3e5578 Compare January 4, 2025 19:15
@jchenga jchenga force-pushed the 3318-configuration-for-connection-pools-limit branch from e3e5578 to 420dd1c Compare January 4, 2025 19:18
@jchenga
Copy link
Contributor Author

jchenga commented Jan 4, 2025

I have made the suggested changes.

/**
* Specifies the maximum number of connection pools that the provider can create.
* If the number of connection pools created exceeds this value, a warning message is logged.
* The value must be positive or zero; otherwise, the connection pools check is ignored.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to allow this value to be set to 0? If one doesn't want pooling, one can disable the pooling.

* @return the current {@link Builder} instance with the updated configuration.
*/
public Builder maxConnectionPools(int maxConnectionPools) {
this.maxConnectionPools = maxConnectionPools;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a check for the provided value.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want the user to be able to reset this to -1? For example:

/**
* Set the options to use for configuring {@link ConnectionProvider} the maximum number of registered
* requests for acquire to keep in a pending queue
* When invoked with -1 the pending queue will not have upper limit.
* Default to {@code 2 * max connections}.
*
* @param pendingAcquireMaxCount the maximum number of registered requests for acquire to keep
* in a pending queue
* @return {@literal this}
* @throws IllegalArgumentException if pendingAcquireMaxCount is negative
*/
public final SPEC pendingAcquireMaxCount(int pendingAcquireMaxCount) {
if (pendingAcquireMaxCount != -1 && pendingAcquireMaxCount <= 0) {
throw new IllegalArgumentException("Pending acquire max count must be strictly positive");
}
this.pendingAcquireMaxCount = pendingAcquireMaxCount;
return get();
}

}
}
finally {
disposableServer.dispose();
Copy link
Member

Choose a reason for hiding this comment

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

Let's dispose the ConnectionProvider here.
No need to dispose the server it will be disposed by reactor.netty.BaseHttpTest#disposeServer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide ConnectionProvider configuration for limiting the number of the connection pools
2 participants