Configure retries for trino-filesystem-gcs

[elonazoulay]

Description

Resolves #19943

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[elonazoulay]

Notes:
Tried different settings for the retries and optimized for the lowest amount of requests, i.e. maxAttempts.
Other settings that worked:

• Only set gcs.retry.max-attempts: 200 - This works has no back off and issues more calls than the default settings

Max attempts < 20 and delay multiplier < 2.0 also resulted in 429's.

Also: the retry strategy needs to be uniform and the trino-filesystem api ensures that the idempotency requirements are not violated - see retries

[ebyhr]

/test-with-secrets sha=b932b90bcfadfeaf7cea8fc940a5b39f00af0b7f

[github-actions]

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/7096161114

[elonazoulay]

Looks like the failure above was unrelated, trying again.

[elonazoulay]

/test-with-secrets sha=b932b90bcfadfeaf7cea8fc940a5b39f00af0b7f

[elonazoulay]

/test-with-secrets sha=52edc07ebeaef3375db3a88049033e30287b938a

[electrum]

Let's remove this config. Users just care that it works, so we should make it work automatically. After reading that page, I'm still not sure why you would change it from UNIFORM to DEFAULT, so I doubt any users would either.

[electrum]

Looks good overall. We definitely need this as the tests are flaky now (and might mean that queries will also fail).

[electrum]

I'll merge this for now since it's in good shape and fixes the frequent test failures. We can follow up on the comments.

[electrum]

I'm confused by the purpose of assertThatNoException(). Any exception should fail the test. Isn't that the normal behavior of unit tests?

[losipiuk]

I think it captures intent nicely - for the case when you just want to check code runs fine and do not do any assertions.
Otherwise, if there is code without assertions in test method, it kinda looks that test writer forgot about sth.

[elonazoulay]

Thanks! I will push a follow up pr with the remaining suggestions to reduce the time and add some comments to explain the test.

[mosabua]

@elonazoulay are you sending docs? And does this need a release notes entry?