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 tcp_keepalive to Config object #2766

Merged
merged 1 commit into from
Sep 29, 2022
Merged

Conversation

dlm6693
Copy link
Contributor

@dlm6693 dlm6693 commented Sep 26, 2022

This PR partially addresses #2249. We have chosen not to incorporate duration_seconds as it is a pretty niche, rarely used argument that also relies on others to be set in the same config object. Because tcp_keepalive is a setting that originates from an operating system, it can only ever be enabled not disabled. This means if set in a scoped config, but not the client config object, it will still be enabled.

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2022

Codecov Report

Base: 95.25% // Head: 95.25% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (a79e9d0) compared to base (5aa876e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2766   +/-   ##
========================================
  Coverage    95.25%   95.25%           
========================================
  Files           62       62           
  Lines        12899    12900    +1     
========================================
+ Hits         12287    12288    +1     
  Misses         612      612           
Impacted Files Coverage Δ
botocore/config.py 100.00% <ø> (ø)
botocore/args.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@jonemo jonemo left a comment

Choose a reason for hiding this comment

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

This makes sense to me. I added a few small suggestions in the inline comments.

Copy link
Contributor

@jonemo jonemo left a comment

Choose a reason for hiding this comment

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

LGTM, but can you double check what is causing the codecov to report a decrease in coverage? I can't figure out a way to see the affected files in their UI, so this might require generating two reports for before and after locally...

@dlm6693 dlm6693 merged commit 409b404 into boto:develop Sep 29, 2022
@dlm6693 dlm6693 deleted the config-keepalive branch September 29, 2022 23:16
aws-sdk-python-automation added a commit that referenced this pull request Sep 30, 2022
* release-1.27.84:
  Bumping version to 1.27.84
  Update to latest endpoints
  Update to latest models
  add `tcp_keepalive` to Config object (#2766)
@miketheman
Copy link
Contributor

miketheman commented Oct 2, 2022

Thanks for working on this! I spent some time to try and see how a client instance could expose the value back to the caller, but it seems like no matter what, it'll always return None:

import boto3
from botocore.config import Config

# No setting set in either config file or Config object
client = boto3.client("dynamodb")
print(client.meta.config.tcp_keepalive)
# None

# Pass in explicitly to enable
client = boto3.client("dynamodb", config=Config(tcp_keepalive=True))
print(client.meta.config.tcp_keepalive)
# None

Is there another way to access the property, or is that this value is not being persisted to the Config object returned?

new_config = Config(**config_kwargs)

I can confirm (via interactive debugger) that the file loader still ends up on scoped_config and the code-side Config object ends up with the correct value in the client_config, so final_args has the right value, but I can't find a way to access it to print out.

miketheman added a commit to miketheman/botocore that referenced this pull request Oct 2, 2022
The `tcp_keepalive` parameter was added to the Config object in boto#2766
When inspecting the return value of an instantiated Client's `meta` with the config, it is always `None`.

Does not handle resolving whether the value has been set in `scoped_config`.
@dlm6693
Copy link
Contributor Author

dlm6693 commented Oct 3, 2022

@miketheman thanks for flagging this! I did a little bit of digging and this should be a one line fix. I'll touch base internally with my team about this just to make sure we want to expose this in the new config object that gets constructed in get_client_args. In the meantime, while you can't access the tcp_keepalive setting directly, you can infer it by examining your client's socket options:

import boto3
from botocore.config import Config
cfg = Config(tcp_keepalive=True)
client = boto3.client('s3', config=cfg)
print(client._endpoint.http_session._socket_options)
[(6, 1, 1), (65535, 8, 1)]

@miketheman
Copy link
Contributor

Thanks @dlm6693 ! That is helpful, for sure. I opened #2771 - uncertain if that's the right solution or not.

@dlm6693
Copy link
Contributor Author

dlm6693 commented Oct 3, 2022

@miketheman sweet! That's exactly the change I was thinking of thank you for working on it. I'll be speaking with my team this afternoon, but assuming we're set, I can approve it and we can get it merged ASAP.

dlm6693 pushed a commit that referenced this pull request Oct 3, 2022
The `tcp_keepalive` parameter was added to the Config object in #2766
When inspecting the return value of an instantiated Client's `meta` with the config, it is always `None`.

Does not handle resolving whether the value has been set in `scoped_config`.
@HayesData
Copy link

thanks @dlm6693 and @miketheman my team is dealing with this exact issue right now and getting an official fix in place would be mega helpful

@miketheman
Copy link
Contributor

@HayesData Sorry to hear you're hitting this issue! An "official fix" is already in place with this change since version 1.27.84 - the change allows setting the desired configuration via Config instantiation, not an environment variable (yet).

Like @dlm6693 posted a little earlier, you can update your client creation to account for the configuration already, like so:

import boto3
from botocore.config import Config
cfg = Config(tcp_keepalive=True)  # here's the important bit!
s3_client = boto3.client('s3', config=cfg)
s3_client....

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.

5 participants