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

fix: Add retries to SAR service calls on throttle #2352

Merged
merged 3 commits into from
Apr 4, 2022

Conversation

mildaniel
Copy link
Contributor

Issue #, if available:

Description of changes:

Description of how you validated changes:

Checklist:

  • Add/update unit tests using:
  • Add/update integration tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

from enum import Enum

LOG = logging.getLogger(__name__)


class SamPlugins(object):
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move this to a separate file to resolve a circular dependency.
samtranslator.plugins (__init__.py) -> samtranslator.model.exceptions… -> samtranslator.plugins.LifeCycleEvents

@codecov-commenter
Copy link

Codecov Report

Merging #2352 (4bfc56b) into develop (e7a1496) will increase coverage by 0.73%.
The diff coverage is 98.47%.

@@             Coverage Diff             @@
##           develop    #2352      +/-   ##
===========================================
+ Coverage    93.58%   94.31%   +0.73%     
===========================================
  Files           90       98       +8     
  Lines         6124     7127    +1003     
  Branches      1260     1468     +208     
===========================================
+ Hits          5731     6722     +991     
- Misses         183      196      +13     
+ Partials       210      209       -1     
Impacted Files Coverage Δ
samtranslator/model/api/api_generator.py 94.05% <92.85%> (-0.31%) ⬇️
samtranslator/__init__.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/dialup.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/feature_toggle.py 100.00% <100.00%> (+12.16%) ⬆️
samtranslator/intrinsics/actions.py 98.78% <100.00%> (+0.06%) ⬆️
samtranslator/intrinsics/resource_refs.py 95.83% <100.00%> (-0.17%) ⬇️
samtranslator/metrics/method_decorator.py 100.00% <100.00%> (ø)
samtranslator/metrics/metrics.py 100.00% <100.00%> (ø)
samtranslator/model/__init__.py 97.63% <100.00%> (-0.02%) ⬇️
samtranslator/region_configuration.py 77.77% <0.00%> (-22.23%) ⬇️
... and 46 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 d796f30...4bfc56b. Read the comment docs.

def _make_service_call_with_retry(self, service_call, app_id, semver, key, logical_id):
call_succeeded = False
start_time = time()
while (time() - start_time) < self.TEMPLATE_WAIT_TIMEOUT_SECONDS:
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a similar retry pattern used in on_after_transform_template. I can see things are done a bit differently within this retry and that retry. But can we try to refactor the retry part into a decorator that can be used on both?

Copy link
Contributor

Choose a reason for hiding this comment

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

both on_after_transform_template && on_before_transform_template functions at the end they call _sar_service_call to do the SAR service call, so I think we can add our retry logic in _sar_service_call call.

But we need to make sure that the waiting time we will use in retry is cumulative between all retials in the different paths, and this cumulative value should be less than TEMPLATE_WAIT_TIMEOUT_SECONDS

@mildaniel mildaniel merged commit a3c40bb into aws:develop Apr 4, 2022
@mildaniel mildaniel deleted the retry-sar-calls branch April 4, 2022 16:15
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.

None yet

4 participants