-
Notifications
You must be signed in to change notification settings - Fork 29
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
fix: include more info for retry token bucket capacity errors #1217
Conversation
{ | ||
"id": "3456d00f-1b29-4d88-ab02-045db1b1ebce", | ||
"type": "bugfix", | ||
"description": "Include more information when retry strategy halts early due to capacity errors", |
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.
Suggestion: Include more information when retry strategy halts early due to **retry token bucket** capacity errors
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.
Good idea. 👍
* Set if there are additional context elements about the error | ||
*/ | ||
public val AdditionalClientContext: AttributeKey<List<ClientErrorContext>> = | ||
AttributeKey("aws.smithy.kotlin#AdditionalClientContext") |
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.
Suggestion: AdditionalClientErrorContext
& "aws.smithy.kotlin#AdditionalClientErrorContext"
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'd rather not add Error
to these two identifiers since they're already explicitly part of the ErrorMetadata
class. Are you concerned about users or maintainers not having enough information about what the attribute does or how to use it?
The names already feel too long and in fact I was contemplating removing the word Additional
as well:
public val ClientContext: AttributeKey<List<ClientErrorContext>> =
AttributeKey("aws.smithy.kotlin#ClientContext")
How would you feel about that?
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 agree with removing the Additional
part of the name. I see what you're saying about it being in ErrorMetadata
. Initially it seemed odd to me that the type of the attribute is ClientErrorContext
but the key is AdditionalClientContext
, without Error
in the name. Perhaps another suggestion is ClientErrorContext
. I don't feel strongly about this though
Issue #
Resolves awslabs/aws-sdk-kotlin#1321
Description of changes
This change adds more information to exceptions thrown from the standard retry strategy when circuit breaker mode is enabled and the token bucket is exhausted. Exceptions will now include an additional element in the message such as:
"<service exception message>, Early retry termination: Insufficient client capacity to attempt retry; halting on attempt 2 of 3"
To accomplish this, this PR adds a new
AdditionalClientContext
attribute toServiceException
which holds a list of zero or more elements. These additional context (if present) are appended to service exception messages.StandardRetryStrategy
now sets this additional context when throwing an exception due to insufficient bucket capacity while in circuit breaker mode. This decouples the exception from knowing about the various types of context.Companion PR: awslabs/aws-sdk-kotlin#1498
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.