Skip to content

Conversation

tobywf
Copy link
Contributor

@tobywf tobywf commented Dec 13, 2019

Issue #, if available:

Description of changes: Static HTTP client to prevent new connection pools each request.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@tobywf tobywf self-assigned this Dec 13, 2019
Copy link
Contributor

@rjlohan rjlohan left a comment

Choose a reason for hiding this comment

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

Looks good. I still question 16 retries though. Think it might blow past available runtime if we actually retried that many times with exponential backoff.


public abstract class AmazonWebServicesProvider {

protected static final ClientOverrideConfiguration CONFIGURATION = ClientOverrideConfiguration.builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the available runtime, is 16 retries too many?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ported it as is, but happy to tweak this if we have better values (the defaults?).

Choose a reason for hiding this comment

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

+1, default is 3 times I think (except 10 for DDB client). 16 seems too much.

Choose a reason for hiding this comment

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

Also, since all aws clients are sharing the same HTTP client, do we want to increase the Maximum Connections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think 50 is fine, right? Given that the Lambda in each VM only really serves one request at a time (even if we create a few more connections for every request).

@tobywf tobywf marked this pull request as ready for review December 13, 2019 23:19
@tobywf
Copy link
Contributor Author

tobywf commented Dec 13, 2019

I'll be exercising handlers based on this all weekend, but so far it's looking promising. The heap size isn't growing, GC is happening, and it looks fine. I'm somewhat confident that this won't accidentally share credentials (the HTTP client doesn't use AWS credentials, so having a long-lived HTTP client should be safer than having long-lived clients).

@tobywf
Copy link
Contributor Author

tobywf commented Dec 16, 2019

Looking good. Will merge on Monday once it's ran all weekend without issues.

@johnttompkins
Copy link
Contributor

may want to also bump the java lib?

@tobywf
Copy link
Contributor Author

tobywf commented Dec 16, 2019

I was going off #213 , which is also linked in the runbook. Is that PR incomplete/what else needs to be done?

@johnttompkins
Copy link
Contributor

I was going off #213 , which is also linked in the runbook. Is that PR incomplete/what else needs to be done?

Since there are changes to the java library we'd need something like [this] (aws-cloudformation/cloudformation-resource-schema#63). The previous PR only had changes to the python lib & the wrapper java library stayed the same, so there was no reason to bump.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants