-
Notifications
You must be signed in to change notification settings - Fork 57
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
[FLINK-36357][Connectors/Kinesis] Set up AWS SDK default retry strate… #170
Conversation
…gy configurations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -254,31 +252,10 @@ private RetryStrategy createExpBackoffRetryStrategy( | |||
final BackoffStrategy backoffStrategy = | |||
BackoffStrategy.exponentialDelayHalfJitter(initialDelay, maxDelay); | |||
|
|||
return StandardRetryStrategy.builder() | |||
return SdkDefaultRetryStrategy.standardRetryStrategyBuilder() | |||
.backoffStrategy(backoffStrategy) | |||
.throttlingBackoffStrategy(backoffStrategy) | |||
.maxAttempts(maxAttempts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circuit breaker is enabled in this strategy by default. Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering that - is there a reason we really don't want to enable circuit breaker for our setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. I believe it will be useful, we just need to document this as a new change as it might be surprising for users.
@@ -254,31 +252,10 @@ private RetryStrategy createExpBackoffRetryStrategy( | |||
final BackoffStrategy backoffStrategy = | |||
BackoffStrategy.exponentialDelayHalfJitter(initialDelay, maxDelay); | |||
|
|||
return StandardRetryStrategy.builder() | |||
return SdkDefaultRetryStrategy.standardRetryStrategyBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any thoughts on how this would work with a user defined retryable exceptions improvement like the one in the old connector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any thoughts on how this would work with a user defined retryable exceptions improvement like the one in the old connector?
Good qn - we can add more predicates by calling retryOnException()
, but we won't be able to override the ones already marked as non-retryable in the SDK defaults, because I think it does eager checks for non-retryable errors.
…gy configurations
Purpose of the change
Configure SDK retry default configurations. This helps with classification of retryable errors out of the box.
Verifying this change
Significant changes
(Please check any boxes [x] if the answer is "yes". You can first publish the PR and check them afterwards, for convenience.)
@Public(Evolving)
)