Skip to content

AWS system test example_dynamodb_to_s3: add retry when fecthing the export time#31388

Merged
o-nikolas merged 7 commits intoapache:mainfrom
aws-mwaa:vincbeck/system-test/dynamodb
May 24, 2023
Merged

AWS system test example_dynamodb_to_s3: add retry when fecthing the export time#31388
o-nikolas merged 7 commits intoapache:mainfrom
aws-mwaa:vincbeck/system-test/dynamodb

Conversation

@vincbeck
Copy link
Contributor

AWS Dynamodb UpdateContinuousBackups API is not stable and we need to add a retry logic when calling update_continuous_backups. The API might return Backups are being enabled for the table: <...>. Please retry later.

As long as I dont like doing that, we have no choice since the API behavior is not stable on the service side ...


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Contributor

@vandonr-amz vandonr-amz left a comment

Choose a reason for hiding this comment

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

ok to let it go like this, but I'd prefer it without the random.

vandonr-amz
vandonr-amz approved these changes May 23, 2023
@vincbeck
Copy link
Contributor Author

Thanks for the feedbacks guys, it made me look if any library was already used for such purpose and there is one: tenacity. I removed the custom implementation and now use this library. It should address all your concerns.

Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

Approved, but I'll hold off merging since there is a broken static check making the build go red.

@ferruzzi
Copy link
Contributor

@o-nikolas and @vincbeck the CI is green on this one, do you need another review on it or is it good to go?

@o-nikolas
Copy link
Contributor

@o-nikolas and @vincbeck the CI is green on this one, do you need another review on it or is it good to go?

Nope, I've just been merging/retrying/waiting for it to be green today. I'll merge now :)

@o-nikolas o-nikolas merged commit aac2f8f into apache:main May 24, 2023
@vandonr-amz vandonr-amz deleted the vincbeck/system-test/dynamodb branch May 24, 2023 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants