feat: adds clientside retries #66
Conversation
| RetryStrategy: NewStaticRetryStrategy(&RetryStrategyProps{ | ||
| RetryableRequestStatuses: defaultRetryableStatusCodes, | ||
| MaxRetries: defaultMaxRetries, | ||
| PerRetryTimeout: overallRequestTimeout / defaultMaxRetries, |
There was a problem hiding this comment.
idk if we want to introduce this yet? it's a little different from the other sdks, was just planning on sticking to the most basic FixedCount strategy for now and circling back later.
| } | ||
|
|
||
| type RetryStrategy interface { | ||
| // GetRetryableRequestStatuses Which status codes to retry requests on |
There was a problem hiding this comment.
in the other sdks this interface just has one method
|
|
||
| type RetryStrategyProps struct { | ||
| // Which status codes to retry requests on | ||
| RetryableRequestStatuses []codes.Code |
There was a problem hiding this comment.
you might want to take a look at how i did this in .NET SDK. We need to be able to decide based on status and the request type, so I created a RetryEligibilityStrategy interface that can be re-used across different retry strategies:
| grpc.WithTransportCredentials(credentials.NewTLS(config)), | ||
| grpc.WithUnaryInterceptor(interceptor.AddHeadersInterceptor(authToken)), | ||
| grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor([]grpc_retry.CallOption{ | ||
| grpc_retry.WithMax(request.Config.GetRetryStrategy().GetMaxRetries()), |
There was a problem hiding this comment.
it looks like maybe you are trying to use an off-the-shelf one here? That's appealing but in most of the other languages I'm finding that the off-the-shelf ones can't actually handle our use case (e.g. no way to specify whether to retry based on request type, and we don't want to retry non-idempotent operations)
There was a problem hiding this comment.
also FWIW I haven't necessarily been adding retries for control plane in some of the other sdks for now, but i don't feel strongly about that
| grpc.WithStreamInterceptor(grpc_retry.StreamClientInterceptor([]grpc_retry.CallOption{ | ||
| grpc_retry.WithMax(request.Config.GetRetryStrategy().GetMaxRetries()), | ||
| grpc_retry.WithPerRetryTimeout(request.Config.GetRetryStrategy().GetPerRetryTimeout()), | ||
| grpc_retry.WithCodes(request.Config.GetRetryStrategy().GetRetryableRequestStatuses()...), |
There was a problem hiding this comment.
what does retry mean for a stream interceptor?
| grpc.WithUnaryInterceptor(grpc_retry.UnaryClientInterceptor([]grpc_retry.CallOption{ | ||
| grpc_retry.WithMax(request.Config.GetRetryStrategy().GetMaxRetries()), | ||
| grpc_retry.WithPerRetryTimeout(request.Config.GetRetryStrategy().GetPerRetryTimeout()), | ||
| grpc_retry.WithCodes(request.Config.GetRetryStrategy().GetRetryableRequestStatuses()...), |
There was a problem hiding this comment.
same thoughts here about off-the-shelf and support for configuring by request type
Closes #44
Closes #49