Skip to content

Use a Retryer instead of retry loops for DynamoDB #1152

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

Closed
bboreham opened this issue Jan 2, 2019 · 6 comments
Closed

Use a Retryer instead of retry loops for DynamoDB #1152

bboreham opened this issue Jan 2, 2019 · 6 comments
Assignees
Labels
component/aws keepalive Skipped by stale bot size/medium storage/chunks Chunks storage engine type/chore Something that needs to be done; not a bug or a feature

Comments

@bboreham
Copy link
Contributor

bboreham commented Jan 2, 2019

The DynamoDB client disables retries and then wraps everything in a loop to do retries.
Disabling retries came in #153; the backoff loop has gone through many iterations since #119.

The stated reason is that we want to monitor retries.

From my experiences using the DynamoDB API elsewhere, I think it would be far cleaner to implement a Retryer which adds the required monitoring, let the AWS SDK do retries, and take out the explicit loops.

Noted at #792 (comment).

@rlisagor
Copy link
Contributor

There's a chance I can take this on, depending on our priorities. It seems like the logic in the DefaultRetryer from the AWS SDK should work, if wrapped with monitoring code. Would that be sufficient? Or do you envision some custom retry logic?

@csmarchbanks
Copy link
Contributor

I believe the intention is to just wrap with monitoring code and stop trying to do any custom retry logic.

@stale
Copy link

stale bot commented Feb 3, 2020

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 3, 2020
@pracucci pracucci added keepalive Skipped by stale bot and removed stale labels Feb 3, 2020
@pracucci
Copy link
Contributor

@bboreham In the PR #2452 you mentioned it partially fixes this issue. What's left out?

@bboreham
Copy link
Contributor Author

I believe all of pkg/chunk/aws/dynamodb_table_client.go could be changed from the loop in backoffAndRetry() to the retryer added in #2452.

BatchWrite() and getDynamoDBChunks() also implement their own retry loops, but these have complicated interactions with throttling so I suspect will have to stay.

@bboreham bboreham added the storage/chunks Chunks storage engine label Jun 10, 2021
@alanprot
Copy link
Member

Chunks storage was removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/aws keepalive Skipped by stale bot size/medium storage/chunks Chunks storage engine type/chore Something that needs to be done; not a bug or a feature
Projects
None yet
Development

No branches or pull requests

6 participants