AWS max retries option added

[RicardoLorenzo]

Adding an option for defining the maximum number of AWS request retries and defaulting to 3 as per AWS documentation.

[deepthi]

Have you already tested this? There are no integration tests against s3.

[rafael]

From my perspective, this looks good. Given that this is setting the default value to the same value that the golang driver already uses: https://sourcegraph.com/github.com/aws/aws-sdk-go@39208f40a1ca0c02a3b7a950fcc78db3ea50cf25/-/blob/aws/client/default_retryer.go#L40

Effectively, this PR is a noop that exposes a way to tune this value from vitess.

@deepthi I think this a bit tricky to test as the failures we see here are transient. Also, we will be testing that S3 driver is doing what it advertises, so I don't think adding a test here is required.

What kind of test you had in mind?

[deepthi]

I see the point about it being hard to test. But if it is supposed to default to 3 retries, why wasn't it being retried? (I looked at the code you linked to and agree with your analysis that it is supposed to default to 3)

[rafael]

@deepthi I believe they are being retried. My thinking is that they are still failing after three attempts.

I think more than the trickiness, what I wanted to convey is that this change only adds more flexibility when configuring the driver. I'm thinking that adding a test for this is to test that the external dependency is respecting the contract. That seems like an anti-pattern.

[demmer]

Although we don't need tests for something like this, I also think we shouldn't override the defaults unless the user specifies a parameter.

So, a better change would be to make the flag default to -1 (meaning the value is unset) and then only set the value in the config if it's 0 or greater.

That way we're not changing existing behavior in potentially subtle ways.

[rafael]

Ah this is a good callout. @RicardoLorenzo, here an example of how we have done that in the past: #4037

[RicardoLorenzo]

I'm sorry, I was sick last week and just seeing the comments now. I will look into this.

[RicardoLorenzo]

I have implemented the proposed change.

[deepthi]

I'm thinking that adding a test for this is to test that the external dependency is respecting the contract. That seems like an anti-pattern.

No need to test for that, my intention was to ask if this had already been sanity-tested.
I have no objection to the change itself.

[rafael]

Looks good. Could you squash the commits? I think for this PR it would be better just to have a single one.

[RicardoLorenzo]

Good point! I have combined the commits into 1 as suggested.

[rafael]

LGTM +1