Add ability to set max retry attempts #1260

kyleknap · 2017-08-10T00:01:34Z

This adds the ability for users to set the max retry attempts for any client call through the botocore.config.Config object. So for example:

from botocore.session import Session
from botocore.config import Config

s = Session()
# Will retry any method call at most 10 times
c = s.create_client('s3', config=Config(retries={'max_attempts': 10}))

It addresses the following issues:

Fixes #482
Fixes #882
Fixes boto/boto3#1104

There are a couple of related PR's for this as well:
#1236
#891

I think I would prefer to go with the approach proposed in this PR because it exposes a little less of the implementation details of how retries work in botocore (so we can swap the strategy out potentially in the future if needed) and allows for retry configuration to be expanded by making a general retries parameter in the Config object.

kyleknap · 2017-08-10T00:05:43Z

botocore/client.py

@@ -70,6 +70,7 @@ def create_client(self, service_name, region_name, is_secure=True,
            service_model, region_name, is_secure, endpoint_url,
            verify, credentials, scoped_config, client_config, endpoint_bridge)
        service_client = cls(**client_args)
+        self._register_retries(service_client)


Might be worth double checking that moving when the retries gets registered is fine. Now, it is now getting registered to the client's event emitter after the client is created instead of on any service model load and to the client creator's event emitter. I made the change because it is technically more correct (in terms of what event emitter the handler should be registered to), the code flow was better (we are using the computed client config instead of what is provided directly to the create_client() call), and it should not affect any of the other registered handlers.

kyleknap · 2017-08-10T00:16:44Z

Hmm looks like there is a bad patch somewhere as an unrelated test is failing. Going to have to figure that out as I use a context manager for the patches I use.

Even prior to this feature, when a retry config was built if a service had specific configurations it would mutate the cached retry model. So for example if a user created a dynamodb client and then an s3 client, the s3 client would inherit the max attempts of 10 from the dynamodb client. Tests were also added for checking that the max attempts provided to the session can be overriden at the client level.

codecov-io · 2017-08-10T19:40:45Z

Codecov Report

Merging #1260 into develop will decrease coverage by 1.41%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #1260      +/-   ##
===========================================
- Coverage    98.04%   96.62%   -1.42%     
===========================================
  Files           45       45              
  Lines         7361     7382      +21     
===========================================
- Hits          7217     7133      -84     
- Misses         144      249     +105

Impacted Files	Coverage Δ
botocore/args.py	`100% <ø> (ø)`	⬆️
botocore/handlers.py	`95.83% <ø> (-0.5%)`	⬇️
botocore/config.py	`100% <100%> (ø)`	⬆️
botocore/exceptions.py	`100% <100%> (ø)`	⬆️
botocore/client.py	`99.73% <100%> (ø)`	⬆️
botocore/translate.py	`100% <100%> (ø)`	⬆️
botocore/compat.py	`57.77% <0%> (-34.08%)`	⬇️
botocore/awsrequest.py	`74.09% <0%> (-24.55%)`	⬇️
botocore/hooks.py	`94.52% <0%> (-1%)`	⬇️
... and 1 more

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 e3c6fb5...f13e5bf. Read the comment docs.

kyleknap · 2017-08-10T19:42:22Z

Alright tests now pass. Also, I found a bug in our retry config logic. So I fixed that while I was looking into the failures as they were related. Should be good to look at.

JordonPhillips

You should make sure it doesn't get clobbered by this. That only gets called through the CLI, which you aren't supporting right now, but still. Might be worth seeing if that can be removed.

JordonPhillips · 2017-08-10T19:50:02Z

botocore/config.py

@@ -157,6 +173,12 @@ def _validate_s3_configuration(self, s3):
                raise InvalidS3AddressingStyleError(
                    s3_addressing_style=addressing_style)

+    def _validate_retry_configuration(self, retries):


Should probably also validate somewhere that max_attempts is >= 0.

Sounds good. I can add that.

kyleknap · 2017-08-10T20:53:39Z

I checked for that handler. I did not see it being used when I sent it up the PR. I will look to see if I can remove it. It should be legacy code as that bit of code has roots to when botocore was based on services and operations and not just clients.

Renamed a validation error to include Error at the end and throw validation errors for max_attempts that are smaller than 0.

JordonPhillips · 2017-08-10T21:58:23Z

botocore/exceptions.py

    """Error when invalid retry configuration is specified"""
    fmt = (
        'Cannot provide retry configuration for "{retry_config_option}". '
        'Valid retry configuration options are: \'max_attempts\''
    )

+
+class InvalidMaxRetryAttemptsError(BotoCoreError):


This should probably subclass from InvalidRetryConfigurationError

Sure that's fine with me.

jamesls

Looks straightforward to me. Just had a few questions.

jamesls · 2017-08-10T21:39:26Z

botocore/exceptions.py

+    """Error when invalid retry configuration is specified"""
+    fmt = (
+        'Value provided to "max_attempts": {provided_max_attempts} must '
+        'be an integer greater than zero.'


greater than or equal to.

jamesls · 2017-08-10T21:50:01Z

botocore/translate.py

+    # retry model has no chance of getting mutated when the service specific
+    # configuration or client retry config is merged in.
+    final_retry_config = {
+        '__default__': copy.deepcopy(retry_model.get('__default__', {}))


So was this deepcopy always a bug then that we just never ran into in practice until now?

Yeah there was a bug. The only time it would be executed though is if you created a dynamodb client and then created other types of clients after as the other clients would inherit the 10 max_attempts of dynamodb. I added the test_service_specific_defaults_do_not_clobber to make sure that does not happen again.

jamesls · 2017-08-10T22:26:38Z

tests/functional/test_retry.py

+        # for this service.
+        self.assert_will_retry_n_times(client.list_repositories, 4)
+
+    def test_service_specific_defaults_do_not_clobber(self):


Can you explain what this is actually testing? It looks like it's just verifying we're able to load service specific config. I don't really get the clobbering part.

This is to test the case:

c = session.create_client('dynamodb') c2 = session.create_client('ec2')

The second client for ec2 would actually have the max retries of dynamodb, even though it is configured for 5 max_attempts.

I also see what I can do to make the name clearer.

yeah or just a comment or something to explain it.

jamesls · 2017-08-10T22:33:13Z

botocore/translate.py

+        # most. So to translate this number from the client config, one is
+        # added to convert it to the maximum number request that will be made
+        # by including the initial request.
+        retry_config['__default__'][


We don't support this now, but if we ever support max_attempts at the operation level, we'll need to revisit how we do this. Might be worth adding a note about that.

I can do that.

Mainly added comments, renamed some methods, and changed the inheritance of an exception class

JadyNews · 2018-02-09T03:23:38Z

There is an issue with sts
init() got an unexpected keyword argument 'aws_session_token'

`import boto3
import botocore
from boto3.session import Session
from botocore.config import Config

s = Session()
sts_client = s.create_client('sts', config=Config(retries={'max_attempts': 10}))

def assumerole(arn):
try:
assumedRoleObject = sts_client.assume_role(
RoleArn=arn,
RoleSessionName=socket.gethostname()
)
credentials = assumedRoleObject['Credentials']
session = Session (
aws_access_key_id = credentials['AccessKeyId'],
aws_secret_access_key = credentials['SecretAccessKey'],
aws_session_token = credentials['SessionToken'],
)
return session`

Python/2.7.10
boto3==1.5.25
botocore==1.8.39

Originally [a hack was put in place to disable automatic Lambda retries](boto/boto3#1104) because boto3 lacked support for this. Unfortunately this hack no longer works with the latest boto3 code because it touches internal variables that apparently have been removed or renamed. Fortunately, though, [support for disabling automatic retries has been added](boto/botocore#1260).

kyleknap commented Aug 10, 2017

View reviewed changes

Add ability to set max retry attempts

6361ed1

kyleknap force-pushed the max-attempts branch from 471c961 to 6361ed1 Compare August 10, 2017 00:10

kyleknap requested review from dstufft, jamesls, stealthycoin, joguSD and JordonPhillips August 10, 2017 19:36

JordonPhillips reviewed Aug 10, 2017

View reviewed changes

Improve retry config validation errors

84c7638

Renamed a validation error to include Error at the end and throw validation errors for max_attempts that are smaller than 0.

JordonPhillips reviewed Aug 10, 2017

View reviewed changes

JordonPhillips added the incorporating-feedback label Aug 10, 2017

jamesls reviewed Aug 10, 2017

View reviewed changes

kyleknap added 2 commits August 10, 2017 16:44

Add note why retry handler cannot be removed

cea3473

Made smaller updates based on feedback

c4dc61d

Mainly added comments, renamed some methods, and changed the inheritance of an exception class

JordonPhillips approved these changes Aug 11, 2017

View reviewed changes

kyleknap force-pushed the max-attempts branch from d2964db to 52422d6 Compare August 11, 2017 19:38

Add change log for max retry attempts feature

f13e5bf

kyleknap force-pushed the max-attempts branch from 52422d6 to f13e5bf Compare August 11, 2017 19:38

jamesls approved these changes Aug 11, 2017

View reviewed changes

kyleknap merged commit 32c40dd into boto:develop Aug 11, 2017

kyleknap deleted the max-attempts branch August 11, 2017 20:14

This was referenced Aug 11, 2017

Allow max_attempts to be set through the Config object. #1236

Closed

Add possibility of modifying retry_config #891

Closed

ross mentioned this pull request Aug 12, 2017

Route53 API Rate Exceeded octodns/octodns#96

Closed

ross mentioned this pull request Aug 12, 2017

Add support for increasing Route53 retries octodns/octodns#97

Merged

kislyuk mentioned this pull request Sep 6, 2017

Document retry configuration for rate throttling on log group creation kislyuk/watchtower#47

Open

jantman mentioned this pull request Sep 13, 2017

Configurable exponential timeout #1205

Open

tangerinedream mentioned this pull request Sep 14, 2017

Ability to specify alternative default retry for botocore max_attempts due to RequestLimitExceeded scenarios tangerinedream/AWS_EC2_Scheduler#5

Closed

taraslayshchuk mentioned this pull request Oct 24, 2017

boto2.num_retries equivalent in retryhandler.py or _retry.json #882

Closed

ryantuck mentioned this pull request Apr 27, 2018

Feature request: Disable automatic retries for lambda invoke boto/boto3#1104

Closed

kbroughton mentioned this pull request Jun 19, 2018

Throttle Hit When Using Tool nccgroup/PMapper#1

Closed

jsf9k mentioned this pull request Sep 14, 2018

Remove now-broken hack and replace with proper solution 18F/domain-scan#276

Merged

wjordan mentioned this pull request Oct 20, 2018

Support retries / max_attempts configuration aws/aws-cli#3665

Closed

jantman mentioned this pull request Nov 13, 2019

AWS API Limits causing Throttling and RequestLimitExceeded errors duo-labs/cloudmapper#598

Closed

lukeorland mentioned this pull request Mar 18, 2020

Enable retries for Fargate Agent PrefectHQ/prefect#2170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to set max retry attempts #1260

Add ability to set max retry attempts #1260

kyleknap commented Aug 10, 2017

kyleknap Aug 10, 2017

kyleknap commented Aug 10, 2017

codecov-io commented Aug 10, 2017 •

edited

Loading

kyleknap commented Aug 10, 2017

JordonPhillips left a comment

JordonPhillips Aug 10, 2017

kyleknap Aug 10, 2017

kyleknap commented Aug 10, 2017

JordonPhillips Aug 10, 2017

kyleknap Aug 10, 2017

jamesls left a comment

jamesls Aug 10, 2017

jamesls Aug 10, 2017

kyleknap Aug 10, 2017

jamesls Aug 10, 2017

kyleknap Aug 10, 2017

kyleknap Aug 10, 2017

jamesls Aug 11, 2017

jamesls Aug 10, 2017

kyleknap Aug 10, 2017

JadyNews commented Feb 9, 2018 •

edited

Loading

Add ability to set max retry attempts #1260

Add ability to set max retry attempts #1260

Conversation

kyleknap commented Aug 10, 2017

Choose a reason for hiding this comment

kyleknap commented Aug 10, 2017

codecov-io commented Aug 10, 2017 • edited Loading

Codecov Report

kyleknap commented Aug 10, 2017

JordonPhillips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JadyNews commented Feb 9, 2018 • edited Loading

codecov-io commented Aug 10, 2017 •

edited

Loading

JadyNews commented Feb 9, 2018 •

edited

Loading