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 maxOpenRequests param in DynamoConfig #1545

Merged
merged 3 commits into from Mar 5, 2019
Merged

Add maxOpenRequests param in DynamoConfig #1545

merged 3 commits into from Mar 5, 2019

Conversation

williamho
Copy link
Contributor

Pull Request Checklist

Fixes

Fixes #1542

Purpose

Allows the maxOpenRequests of the underlying host connection pool to
be configured separately from the maxConnections value (as specified
by the parallelism value in the config).

Background Context

To preserve old behavior, I made it an optional field, where if it's
absent, then it will use the parallelism value as before. However the
one thing I'm not sure of is whether I should have the Option exposed
in the public interface (since this is meant to be used in Java as
well). Glad to accept any suggestions on how to handle optional values
like this.

References

#1542 - Can't specify maxOpenRequests in DynamoClient

Allows the `maxOpenRequests` of the underlying host connection pool to
be configured separately from the `maxConnections` value (as specified
by the `parallelism` value in the config).

If unspecified, it will retain the previous behavior, where it's the
same as `maxConnections`.

Fixes #1542
@2m
Copy link
Member

2m commented Mar 1, 2019

Thanks for the PR. It is looking very good. Regarding exposing Option in the public API - it is fine. It serves as a getter for Scala users. However you need to add a getter for Java users called getGaxOpenRequests which will return an Optional.

@williamho
Copy link
Contributor Author

williamho commented Mar 1, 2019

Thanks, that makes sense.

Now that I'm thinking about it though, while maxOpenRequests works for me, I can imagine that in the future, someone else will want to be able to set other fields on the underlying ConnectionPoolSettings.

Rather than adding just one configurable setting, would it be better to just accept the whole ConnectionPoolSettings as an optional param? This way it has maximum flexibility. Not sure if that would leak too much of the implementation details to the public API though.

edit: disregard that, I forgot those other params can still be set with the usual config

Also rename the config field to use hyphens for consistency
@williamho
Copy link
Contributor Author

Actually, disregard my last comment. I forgot that the other connection pool settings can still be set with the usual akka.http.host-connection-pool config.

I've updated the PR to allow getting/setting with java Optionals, as well as using hyphens in the config for consistency with other config fields

Copy link
Member

@2m 2m left a comment

Choose a reason for hiding this comment

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

LGTM

@2m 2m added this to the 1.0-M4/RC1 milestone Mar 5, 2019
@2m 2m merged commit c18bfb0 into akka:master Mar 5, 2019
@2m
Copy link
Member

2m commented Mar 5, 2019

Awesome stuff. Thanks a lot!

@williamho williamho deleted the max-open-requests branch March 5, 2019 14:52
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.

None yet

2 participants