Skip to content

Add tcp_keepalive as AWS_TCP_KEEPALIVE env var#2268

Closed
ryansb wants to merge 1 commit into
boto:developfrom
ryansb:feature/use-config-store-for-tcp-keepalive
Closed

Add tcp_keepalive as AWS_TCP_KEEPALIVE env var#2268
ryansb wants to merge 1 commit into
boto:developfrom
ryansb:feature/use-config-store-for-tcp-keepalive

Conversation

@ryansb

@ryansb ryansb commented Jan 11, 2021

Copy link
Copy Markdown

Related to #2249 - adds tcp_keepalive as an environment variable via the new config_store way of handling variables.

@codecov-io

codecov-io commented Jan 11, 2021

Copy link
Copy Markdown

Codecov Report

Merging #2268 (49529ad) into develop (b0d5a43) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2268      +/-   ##
===========================================
- Coverage    98.25%   98.24%   -0.01%     
===========================================
  Files           58       58              
  Lines        10877    10879       +2     
===========================================
+ Hits         10687    10688       +1     
- Misses         190      191       +1     
Impacted Files Coverage Δ
botocore/configprovider.py 93.37% <ø> (ø)
botocore/args.py 99.40% <50.00%> (-0.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0d5a43...49529ad. Read the comment docs.

@miketheman

Copy link
Copy Markdown
Contributor

cc @dlm6693 This may be of interest

@ryansb ryansb force-pushed the feature/use-config-store-for-tcp-keepalive branch 2 times, most recently from fc74450 to 0a84d1b Compare October 4, 2022 20:02
@ryansb ryansb force-pushed the feature/use-config-store-for-tcp-keepalive branch from 0a84d1b to a7495d8 Compare October 4, 2022 20:03
@devesch

devesch commented Dec 11, 2022

Copy link
Copy Markdown

What's holding this to reach production?

@HayesData

Copy link
Copy Markdown

I spent some time on a conference call /w AWS support last week and they openly acknowledged this needs to go out, would be super helpful if this made it to a stable release as this is the source of a lot of pain for some Lambda applications

@dlm6693

dlm6693 commented Dec 29, 2022

Copy link
Copy Markdown
Contributor

To everyone showing interest in this pull request, we cannot merge this until it can be supported in all AWS SDKs. It's our policy that all AWS_ environment variables are supported in all SDKs.

I will touch base internally with the team and see if this something we can move forward with. Thank you!

@miketheman

miketheman commented Dec 29, 2022

Copy link
Copy Markdown
Contributor

@dlm6693 thanks for the update!
Would you consider a near term specific environment variable? Precedent has been set in the node sdk with a similar value years ago AWS_NODEJS_CONNECTION_REUSE_ENABLED

This could help solve the pain folks have now while we wait for all SDKs to have this ability.

@dlm6693

dlm6693 commented Dec 29, 2022

Copy link
Copy Markdown
Contributor

@miketheman we consider adding it to the client config object as the short term fix. I don't think we'll consider a more specific environment variable as a short term option even with the similar one for JS.

@ronyyosef

Copy link
Copy Markdown

Any news about it?

@RyanFitzSimmonsAK

Copy link
Copy Markdown
Contributor

Hey everyone, thanks for opening this pull request and all the discussion. We're going to close this pull request as not planned. As mentioned earlier, this would need to be a cross-SDK environment variable, and we'd like to avoid further expanding the long list of existing environment variables, particularly for an option that is already supported in the client configuration.

@miketheman

Copy link
Copy Markdown
Contributor

particularly for an option that is already supported in the client configuration.

This kind of response makes me sad.

It assumes that client configuration is as easy to supply as an environment variable, which is most definitely is not.

Thanks for sitting on this for years before deciding that it's not going to happen.

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.

8 participants