DynamoDB: Issue/1682: Adds retries on DynamoDB failed requests as per the DynamoDB documentation #1712
Conversation
* Made retries and its strategy configurable from conf * Added retries using a similar pattern as available on Flow.recoverWithRetries but with TimerGraphStageLogic
…ecated and new versions
* Read retry config from retry path * Renamed parameter name to attempts
…quired and removed duplicate tests
|
Hi @ennru Please review the PR once you can spare some time :) |
|
Hi @2m Build is failing on some Cassandra step which does not concern this PR, how to rerun the build? |
|
Thanks for the PR. We will take a look. It is not a problem with unrelated test failures. We will still merge it, if the failure is of an unrelated test. |
|
Hi @2m The build failure was fixed from the last commit. |
There was a problem hiding this comment.
I think supporting retries for failed requests is really important for DynamoDb connector, as requests can fail quite sporadically with quota exceeded error. Backoff retries really help in that case.
This PR is a great start, and we should focus to have the retry logic decoupled from a particular connector.
| import scala.compat.java8.OptionConverters._ | ||
|
|
||
| @deprecated("Use AwsDynamoSettings instead", "") | ||
| final class DynamoSettings private ( |
There was a problem hiding this comment.
Why was this deprecated in favor of AwsDynamoSettings?
You can add the retrySettings to this class instead.
| case _ @(_: InternalServerErrorException | _: ItemCollectionSizeLimitExceededException | | ||
| _: LimitExceededException | _: ProvisionedThroughputExceededException | | ||
| _: RequestLimitExceededException) => | ||
| Source.single(op).via(opFlow) |
There was a problem hiding this comment.
I think we need to improve the retry logic here. As it currently stands, if the stage fails, the retry will happen with the op that was passed into the flowOp factory at the moment o the flow creation. So it could happen that quite a few operations went through that flow, and then one failed. The retry will happen with the operation that was initially given during the creation of the flow instead of the one, that failed.
I think we should adapt DynamoDb connector flows to be composable with a retry flow in akka-stream-contrib: https://github.com/akka/akka-stream-contrib/blob/master/src/main/scala/akka/stream/contrib/Retry.scala#L42 instead.
| settings.retrySettings.backoffStrategy, { | ||
| case _ @(_: InternalServerErrorException | _: ItemCollectionSizeLimitExceededException | | ||
| _: LimitExceededException | _: ProvisionedThroughputExceededException | | ||
| _: RequestLimitExceededException) => |
There was a problem hiding this comment.
It is possible to set supervision strategy on a stream operator as a stream attribute. We should use that instead so users can customize supervision strategies: https://doc.akka.io/docs/akka/current/stream/stream-error.html#supervision-strategies
|
Thank you for the initial PR. This has sparked a discussion which has materialized a general RetryFlow stage. This PR has been superseded by #1896 where DynamoDb connector is changed to compose with a general RetryFlow. |
Pull Request Checklist
Purpose
To add retry mechanism for failed requests in DynamoDB as per the following document:
Error Handling
References
References #1682
Changes
Background Context
It seemed to be a clean solution without making a lot of changes to the existing codebase.