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

Add java docs to explain how the max connection settings are used #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions HorizonCore/src/main/java/com/hubspot/horizon/HttpConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,37 @@ public static Builder newBuilder() {
return new Builder();
}

/**
* The HttpClient will use this setting to throttle requests.
*
* If the client gets a request, but the max connections are already in use,
* the client will block indefinitely until it has a free connection for the request,
* then make the request once it's able.
*
* The default value is 100.
*/
public int getMaxConnections() {
return maxConnections;
}

/**
* Unlike {@link #getMaxConnections()}, the HttpClient will use this per-host setting to IMMEDIATELY FAIL any requests
* that would cause it to exceed the connections per host limit.
* The client does NOT block / wait for any amount of time if it gets a request when the max connections per host are already used up;
* instead, a {@link org.asynchttpclient.shaded.exception.TooManyConnectionsPerHostException} is thrown from the client immediately.
*
* Note, {@link #getConnectTimeoutMillis()} doesn't come into play in this connection limiting.
* Instead, the internal `acquireTimeout` is the timeout for acquiring a connection permit;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be connect timeout instead?

Copy link
Author

Choose a reason for hiding this comment

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

No, I think what I meant to explain here is that the connect timeout can only come into play after the client has acquired a connection permit, so it's the acquireTimeout defaulting to 0 that causes "The client does NOT block / wait for any amount of time if it gets a request when the max connections per host are already used up" - and since acquireTimeout isn't exposed there's no way to have it not throw immediately when maxConnectionsPerHost is hit. The connect timeout should be for establishing a connection to the API after a connection permit is acquired (technically it could influence connection limiting if it was set way too high, but it isn't part of the immediate failure behavior).
Does that make sense? I could try to reword it if that might help

* this setting isn't exposed and the default is 0.
*
* If you want to use the client to throttle requests indefinitely without throwing exceptions for connection limits,
* use {@link Builder#setMaxConnectionsPerHost(int)} to a higher number than {@link #getMaxConnections()} works
* most of the time (except there's some race condition that can be hit when the client is being totally overwhelmed).
* If possible, it can be better to limit requests to the client.
* For example, it might be possible to limit a thread pool that uses the client accordingly.
*
* The default value is 25.
*/
public int getMaxConnectionsPerHost() {
return maxConnectionsPerHost;
}
Expand Down