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

Remove duplicate retry handler registration #1719

Merged
merged 2 commits into from
May 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion botocore/data/_retry.json
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@
}
},
"kinesis": {
"DescribeStream": {
"__default__": {
"policies": {
"request_limit_exceeded": {
"applies_when": {
Expand Down
55 changes: 0 additions & 55 deletions botocore/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,60 +264,6 @@ def _needs_s3_sse_customization(params, sse_member_prefix):
sse_member_prefix + 'KeyMD5' not in params)


# NOTE: Retries get registered in two separate places in the botocore
# code: once when creating the client and once when you load the service
# model from the session. While at first this handler seems unneeded, it
# would be a breaking change for the AWS CLI to have it removed. This is
# because it relies on the service model from the session to create commands
# and this handler respects operation granular retry logic while the client
# one does not. If this is ever to be removed the handler, the client
# will have to respect per-operation level retry configuration.
def register_retries_for_service(service_data, session,
service_name, **kwargs):
loader = session.get_component('data_loader')
endpoint_prefix = service_data.get('metadata', {}).get('endpointPrefix')
if endpoint_prefix is None:
logger.debug("Not registering retry handlers, could not endpoint "
"prefix from model for service %s", service_name)
return
service_id = service_data.get('metadata', {}).get('serviceId')
if service_id is None:
raise MissingServiceIdError(service_name=service_name)
service_event_name = hyphenize_service_id(service_id)
config = _load_retry_config(loader, endpoint_prefix)
if not config:
return
logger.debug("Registering retry handlers for service: %s", service_name)
handler = retryhandler.create_retry_handler(
config, endpoint_prefix)
unique_id = 'retry-config-%s' % service_event_name
session.register('needs-retry.%s' % service_event_name,
handler, unique_id=unique_id)
_register_for_operations(config, session, service_event_name)


def _load_retry_config(loader, endpoint_prefix):
original_config = loader.load_data('_retry')
retry_config = translate.build_retry_config(
endpoint_prefix, original_config['retry'],
original_config.get('definitions', {}))
return retry_config


def _register_for_operations(config, session, service_event_name):
# There's certainly a tradeoff for registering the retry config
# for the operations when the service is created. In practice,
# there aren't a whole lot of per operation retry configs so
# this is ok for now.
for key in config:
if key == '__default__':
continue
handler = retryhandler.create_retry_handler(config, key)
unique_id = 'retry-config-%s-%s' % (service_event_name, key)
session.register('needs-retry.%s.%s' % (service_event_name, key),
handler, unique_id=unique_id)


def disable_signing(**kwargs):
"""
This handler disables request signing by setting the signer
Expand Down Expand Up @@ -998,7 +944,6 @@ def inject_api_version_header_if_needed(model, params, **kwargs):
('needs-retry.s3.CopyObject', check_for_200_error, REGISTER_FIRST),
('needs-retry.s3.CompleteMultipartUpload', check_for_200_error,
REGISTER_FIRST),
('service-data-loaded', register_retries_for_service),
('choose-signer.cognito-identity.GetId', disable_signing),
('choose-signer.cognito-identity.GetOpenIdToken', disable_signing),
('choose-signer.cognito-identity.UnlinkIdentity', disable_signing),
Expand Down
57 changes: 3 additions & 54 deletions tests/unit/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -552,59 +552,6 @@ def test_run_instances_userdata_blob(self):
'UserData': b64_user_data}
self.assertEqual(params, result)

def test_register_retry_for_handlers_with_no_metadata(self):
no_endpoint_prefix = {'metadata': {}}
session = mock.Mock()
handlers.register_retries_for_service(service_data=no_endpoint_prefix,
session=mock.Mock(),
service_name='foo')
self.assertFalse(session.register.called)

def test_register_retry_for_handlers_with_no_service_id(self):
service_data = {
'metadata': {
'endpointPrefix': 'foo',
},
}
session = mock.Mock(spec=Session)
loader = mock.Mock(spec=Loader)
session.get_component.return_value = loader
service_name = 'foo'
with self.assertRaisesRegexp(MissingServiceIdError, service_name):
handlers.register_retries_for_service(
service_data=service_data,
session=session,
service_name=service_name,
)

def test_register_retry_handlers(self):
service_data = {
'metadata': {
'endpointPrefix': 'foo',
'serviceId': 'foo',
},
}
session = mock.Mock()
loader = mock.Mock()
session.get_component.return_value = loader
loader.load_data.return_value = {
'retry': {
'__default__': {
'max_attempts': 10,
'delay': {
'type': 'exponential',
'base': 2,
'growth_factor': 5,
},
},
},
}
handlers.register_retries_for_service(service_data=service_data,
session=session,
service_name='foo')
session.register.assert_called_with('needs-retry.foo', mock.ANY,
unique_id='retry-config-foo')

def test_get_template_has_error_response(self):
original = {'Error': {'Code': 'Message'}}
handler_input = copy.deepcopy(original)
Expand Down Expand Up @@ -1054,10 +1001,12 @@ def get_handler_names(self, responses):
return names

def test_s3_special_case_is_before_other_retry(self):
client = self.session.create_client('s3')
service_model = self.session.get_service_model('s3')
operation = service_model.operation_model('CopyObject')
responses = self.session.emit(
responses = client.meta.events.emit(
'needs-retry.s3.CopyObject',
request_dict={},
response=(mock.Mock(), mock.Mock()), endpoint=mock.Mock(),
operation=operation, attempts=1, caught_exception=None)
# This is implementation specific, but we're trying to verify that
Expand Down