Conversation
cgonyeo
left a comment
cgonyeo
left a comment
There was a problem hiding this comment.
LGTM. Since the HTTP retry logic is described in the operator's notes, maybe it would be worth adding a sentence or two there on the fact that S3 and TFTP will now retry failed attempts.
|
Ack, thanks for pointing out the docs. I will drop a note there. |
| maxRetries := int(10) | ||
| f.AWSSession, err = session.NewSession(&aws.Config{ | ||
| Credentials: credentials.AnonymousCredentials, | ||
| MaxRetries: &maxRetries, |
There was a problem hiding this comment.
This is slightly more readable as aws.Int(10) if we're hardcoding it anyways.
For reference, this is bumping from the default of 3.
With or without my slight readability nit, this LGTM.
There was a problem hiding this comment.
I am curios how the number 10 was picked though; if you have any information leading to that being a better number than 3 or 5, I'd be interested in hearing it.
I assume the SDK's value was chosen for a reason (especially given all the sdks share 3 for s3, use 10 for certain dynamodb requests, etc etc)
There was a problem hiding this comment.
Bad answer: it's an arbitrary number bumping over the initial default (both for S3 and TFTP). Actual number was a hint from the initial ticket, but that was arbitrary as well. Both clients already do retries so this PR just ups, uniforms and makes the number explicit.
|
Bump on this. Good to go? |
|
Needs to be rebased |
This increases the number of retries performed by the TFTP client in case of transient network errors.
This increases the number of retries performed by the S3 client in case of transient network errors.
This adds a quick operator-note concerning retries of TFTP and S3 downloads.
lucab
force-pushed
the
ups/urls-retries
branch
from
19305e0 to
845cf1a
Compare
|
The original bug was speculative, disproved, and ultimately closed out, so I don't think we need this patch. |
…b.com/stretchr/testify-1.8.3 build(deps): bump github.com/stretchr/testify from 1.8.2 to 1.8.3
6 participants
This increases the number of retries performed by the TFTP and S3 clients in case of transient network errors.
Fixes OST-73