Add amz-sdk-request header, removed amz-sdk-retry header. #2179
Conversation
| return request.toBuilder() | ||
| .putHeader(SDK_RETRY_INFO_HEADER, headerValue) | ||
| .putHeader(SDK_RETRY_INFO_HEADER, "attempt=" + attemptNumber + "; max=" + retryPolicy.numRetries()) |
There was a problem hiding this comment.
I think the retry capacity is very valuable for troubleshooting; otherwise there's no way to tell if the retry gets throttled. I've seen an issue before where all requests failed of IOException, and I initially thought all retry attempts failed, but this header made me realize that the request did not get retried because there was no retry capacity.
There was a problem hiding this comment.
I guess now that we have metrics, we can see it there. It's just not as obvious
There was a problem hiding this comment.
Maybe we could add a debug log whenever we do not retry because of throttling?
millems
force-pushed
the
millem/retry-headers
branch
from
5179a53 to
5d9c56a
Compare
Most changes are from a rename of MockHttpClient to MockSyncHttpClient so that a common parent interface can be added for generic programming purposes. The new amz-sdk-request header does not include the TTL, because it's not at all easy to calculate with pluggable HTTP clients and the specification lists it as optional. The TTL can be added when service teams ask for it.
millems
force-pushed
the
millem/retry-headers
branch
from
5d9c56a to
1cffce8
Compare
| @@ -122,9 +125,17 @@ public boolean shouldRetry(RetryPolicyContext context) { | |||
| context.executionAttributes().putAttribute(LAST_ACQUIRED_CAPACITY, c); | |||
| context.executionAttributes().putAttribute(RETRY_COUNT_OF_LAST_CAPACITY_ACQUISITION, | |||
| context.retriesAttempted()); | |||
| log.trace(() -> "Successfully acquired token bucket capacity to retry this request. " | |||
There was a problem hiding this comment.
This seemed like a bit much for debug, since we're already logging when there wasn't enough capacity. I was thinking if we really needed to debug this, we can ask them to enable trace for this one class.
| /** | ||
| * A set of tests that verify the behavior of retry-related headers (amz-sdk-invocation-id and amz-sdk-request). | ||
| */ | ||
| public abstract class RetryHeaderTestSuite<T extends MockHttpClient> { |
|
SonarCloud Quality Gate failed. |
…2778682e7 Pull request: release <- staging/d25ebd20-7ff7-4143-b9ba-3222778682e7
Most changes are from a rename of MockHttpClient to MockSyncHttpClient so that a common parent interface can be added for generic programming purposes.
The new amz-sdk-request header does not include the TTL, because it's not at all easy to calculate with pluggable HTTP clients and the specification lists it as optional. The TTL can be added when service teams ask for it.