Add sqs client retryer #1099
Add sqs client retryer #1099
Conversation
| @@ -223,7 +222,7 @@ func main() { | |||
| QueueURL: nthConfig.QueueURL, | |||
| InterruptionChan: interruptionChan, | |||
| CancelChan: cancelChan, | |||
| SQS: sqs.New(sess), | |||
| SQS: sqsevent.GetSqsClient(sess), | |||
| ASG: autoscaling.New(sess), | |||
There was a problem hiding this comment.
should we also add retry for asg client?
There was a problem hiding this comment.
We could keep use default retryer for asg client.
| DefaultRetryer: client.DefaultRetryer{ | ||
| NumMaxRetries: client.DefaultRetryerMaxNumRetries, | ||
| MinRetryDelay: client.DefaultRetryerMinRetryDelay, | ||
| MaxRetryDelay: 1200 * time.Millisecond, |
There was a problem hiding this comment.
any particular reason why we don't set MaxRetryDelay to be DefaultRetryerMaxRetryDelay and same for DefaultRetryerMinThrottleDelay? https://github.com/aws/aws-sdk-go/blob/7112c0a0c2d01713a9db2d57f0e5722225baf5b5/aws/client/default_retryer.go#L49C2-L49C29
There was a problem hiding this comment.
Fixed by adding doc comment.
| client.DefaultRetryer | ||
| } | ||
|
|
||
| func (r SqsRetryer) ShouldRetry(req *request.Request) bool { |
There was a problem hiding this comment.
nit: personally I would prefer more descriptive variable names (perhaps retryer instead or r), just so I don't have to search for the type.
| temp bool | ||
| } | ||
|
|
||
| func TestGetSqsClient(t *testing.T) { |
There was a problem hiding this comment.
this test case seems unnecessary because it's testing an implementation detail rather than behavior.
There was a problem hiding this comment.
Would keep test case to test custom retryer config is consumed or not.
Issue #, if available:
#717
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.