Update comments for AWS SDK logging and retryer#1303
Update comments for AWS SDK logging and retryer#1303radoslawszulgo wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates inline documentation in the S3 storage configuration to reflect AWS SDK for Go v2 logging (aws.ClientLogMode) and retryer (aws.Retryer) behavior.
Changes:
- Refreshes
Config.DebugLogLevelscomment to describe supported AWS SDK v2 log mode flags and deprecated v1 aliases. - Refreshes
Config.Retryer/Retryerfield comments to describe v2 retry semantics and backoff controls.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Any sub levels will enable LogDebug level accordingly to AWS SDK Go module behavior | ||
| // https://pkg.go.dev/github.com/aws/aws-sdk-go@v1.40.7/aws#LogLevelType | ||
| // DebugLogLevels enables AWS SDK logging levels. Available options: | ||
| // Signing, Retries, Request, RequestWithBody, Response, ResponseWithBody, DeprecatedUsage, RequestEventMessage |
There was a problem hiding this comment.
The DebugLogLevels comment lists supported values, but it omits ResponseEventMessage, which is accepted by toClientLogMode (and defined in SDKDebugLogLevel). Please update the comment to include ResponseEventMessage so configuration docs match actual behavior.
| // Signing, Retries, Request, RequestWithBody, Response, ResponseWithBody, DeprecatedUsage, RequestEventMessage | |
| // Signing, Retries, Request, RequestWithBody, Response, ResponseWithBody, DeprecatedUsage, RequestEventMessage, ResponseEventMessage |
| // DebugLogLevels enables AWS SDK logging levels. Available options: | ||
| // Signing, Retries, Request, RequestWithBody, Response, ResponseWithBody, DeprecatedUsage, RequestEventMessage | ||
| // and deprecated: LogDebug, HTTPBody, RequestRetries, RequestErrors, EventStreamBody | ||
| // Above values are translated to match the AWS SDK V2: |
There was a problem hiding this comment.
The comment says the listed DebugLogLevels values are "translated to match the AWS SDK V2", but the primary values listed (Signing, Retries, Request, etc.) are already the AWS SDK v2 aws.ClientLogMode flag names; only the deprecated v1 values are mapped for backward compatibility. Consider rewording to clarify that this field is parsed into aws.ClientLogMode and that deprecated v1 names are mapped to the corresponding v2 flags.
| // Above values are translated to match the AWS SDK V2: | |
| // Values are parsed into AWS SDK v2 ClientLogMode; deprecated names are mapped | |
| // to the corresponding v2 flags for backward compatibility: |
| // Retryer is a configuration for aws.Retryer | ||
| // https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Retryer | ||
| // it's implemented with standard retryable, and delay behavior | ||
| // see also: https://github.com/aws/aws-sdk-go-v2/blob/v1.41.5/aws/retry/standard.go |
There was a problem hiding this comment.
The Retryer field comment states "it's implemented with standard retryable, and delay behavior" and links to a specific aws-sdk-go-v2 GitHub tag. In this codebase the implementation is NewCustomRetryer (wrapping retry.NewStandard and enforcing a minimum backoff) and the pinned SDK version may differ over time, so the hardcoded GitHub URL/tag can become stale. Please reword to describe the local implementation (and/or link to aws/retry docs) without tying the comment to a specific upstream tag.
| // Retryer is a configuration for aws.Retryer | |
| // https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Retryer | |
| // it's implemented with standard retryable, and delay behavior | |
| // see also: https://github.com/aws/aws-sdk-go-v2/blob/v1.41.5/aws/retry/standard.go | |
| // Retryer configures the aws.Retryer used by the local custom retryer implementation. | |
| // https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Retryer | |
| // The implementation wraps the AWS SDK standard retryer behavior and applies the | |
| // configured retry delay settings, including enforcing a minimum retry delay. |
| type Retryer struct { | ||
| // Num max Retries is the number of max retries that will be performed. | ||
| // https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/client#DefaultRetryer.NumMaxRetries | ||
| // NumMaxRetries is the number of **additional** max attempts that should be made |
There was a problem hiding this comment.
NumMaxRetries comment uses Markdown formatting ("additional") and has an extra space after the field name. Go doc comments are rendered as plain text; please drop Markdown emphasis and keep formatting consistent (e.g., "NumMaxRetries is the number of additional retry attempts"), matching the NewCustomRetryer behavior of MaxAttempts = numMaxRetries + 1.
| // NumMaxRetries is the number of **additional** max attempts that should be made | |
| // NumMaxRetries is the number of additional retry attempts that should be made |
|
|
||
| // MaxRetryDelay is the maximum retry delay before which retry must be performed. | ||
| // https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/client#DefaultRetryer.MaxRetryDelay | ||
| // MaxRetryDelay is the duration between retried attempts (using ExponentialJitterBackoff) |
There was a problem hiding this comment.
The MaxRetryDelay comment is misleading: it currently reads as if this value is "the duration between retried attempts", but the implementation passes it as retry.StandardOptions.MaxBackoff (a maximum cap on the computed exponential-jitter backoff). Please update the comment to reflect that it is a maximum backoff / maximum retry delay cap, not a fixed delay between attempts.
| // MaxRetryDelay is the duration between retried attempts (using ExponentialJitterBackoff) | |
| // MaxRetryDelay is the maximum backoff / retry delay cap for retried attempts (using ExponentialJitterBackoff) |
No description provided.