Improve retry logic

[oskar-szwajkowski]

Description

This PR improves retries/exception handling around file system operations, additional context can be found in commit messages and in #22678

Changed places in S3/Azure/Gcs filesystems where we throw IOException, and use UnrecoverableIOException whenever it makes sense (it extends from IOException so is backward compatible)

All SDK exceptions are wrapped with UnrecoverableIOException, as third party clients have default retry logic, which we shouldn't further retry whenever it fails

Additional context and related issues

Closes #22678

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.
(X) Release notes are required, with the following suggested text:

* Skip retrying some file system operations that are meant to fail, failing fast instead.

[electrum]

We should remove the retry logic for materialized view fetches. I have no idea why it was added there -- none of the other operations I looked at have it.

[oskar-szwajkowski]

We should remove the retry logic for materialized view fetches. I have no idea why it was added there -- none of the other operations I looked at have it.

Did you mean remove retries from AbstractIcebergTableOperations#refreshFromMetadataLocation or AbstractTrinoCatalog#getMaterializedView ?

or both?

I see that AbstractTrinoCatalog#getMaterializedView is being retried only on MaterializedViewMayBeBeingRemovedException, which is never instantiated so it can be removed rather safely

When it comes to AbstractIcebergTableOperations#refreshFromMetadataLocation, it seems like its retrying on more number of cases, but 20 retries seems like a lot for this operation

[electrum]

If seems fairly arbitrary why many of these exceptions are made non-retriable. For example, why couldn't you retry on a listing failure?

[oskar-szwajkowski]

If seems fairly arbitrary why many of these exceptions are made non-retriable. For example, why couldn't you retry on a listing failure?

I followed the logic that if we catch raw SDK exception, it means that underlying library has failed to retry (which is does by default on exceptions that can be retried), so there is no point trying again

I checked that all AWS/Azure/Gcp clients are set up with default retry policies that we are not overriding, so we have retries for free, trying to add additional on top is just making things fail after more time

Described it more here: #22678 (comment)

[oskar-szwajkowski]

Added new commit on top of previous ones with changing iceberg retries @electrum

[electrum]

"Change iceberg related retries" looks good

[electrum]

I followed the logic that if we catch raw SDK exception, it means that underlying library has failed to retry (which is does by default on exceptions that can be retried), so there is no point trying again

Makes sense. Thanks for explaining.

[electrum]

I checked and we can drop the throws clause from that interface, as none of the implementations throw checked exceptions. We could actually replace it with UnaryOperator<Table>.

[electrum]

The handling needs to be updated, as there is no longer a FailsafeException wrapping something. We should treat these like other Glue exceptions, probably by changing this to

catch (SdkException e) {
    throw new TrinoException(HIVE_METASTORE_ERROR, e);
}

This outer try block can be removed, and this handling moved to the block above.

[electrum]

I don't think this is correct. This retry is in the Glue client and software.amazon.awssdk.services.glue.model.ConcurrentModificationException is a GlueException, so it should be thrown directly from the client.

The original retry code for Glue v1 dropTable and associated test seems to be based on a stack trace after the failure was wrapped in TrinoException, which wouldn't be the case. Although it would be correct for the later added updateTableStatistics as that retry happens outside of the wrapping. The test TestHiveConcurrentModificationGlueMetastore should be updated to throw ConcurrentModificationException directly.

Ideally, we can find an actual exception from the Glue service to validate this.

[oskar-szwajkowski]

I think it is fine, looking at v2 client retries, it uses AwsRetryStrategy#configureStrategy which uses same mechanism, adding some default handling of AwsServiceException classes, so adding handling of another children of this class (ConcurrentModificationException) would just extend this behavior

But Throwables.getRootCause is not needed here, as it can be checked with instanceof directly, as we are not wrapping it with trino exception at this point

[oskar-szwajkowski]

When it comes to v1, it is analogical, com.amazonaws.retry.PredefinedRetryPolicies also check if exception is AmazonServiceException when performing retries, so its not only using it externally to the client, but also internally, so its perfectly fine to check it in lambda, but again, we don't need to check root cause but just plain instanceof should do it

[oskar-szwajkowski]

there is article on that here, that external retries shouldn't be done, but retry customizers should be used instead:
https://docs.aws.amazon.com/codeguru/detector-library/java/aws-custom-retries/

[oskar-szwajkowski]

TestHiveConcurrentModificationGlueMetastore and proxying updateTable does not hold anymore, as those retries are happening deeper than in GlueClient

I think way to test this is to build glue client and assert that retry strategy that is set will yield to try when ConcurrentModificationException is encountered

[oskar-szwajkowski]

I changed second commit Do not rely on Failsafe in glue metastore to test if retry policy is set up correctly on glue client, instead of proxying method with throwing exception, as it doesn't work anymore due to exception being handled deeper

[oskar-szwajkowski]

@electrum can I have re-review after latest changes? I'd like to proceed with this PR

[electrum]

Thanks!

[ebyhr]

@oskar-szwajkowski TestIcebergGlueCatalogConnectorSmokeTest.testDeleteRowsConcurrently looks very flaky after this PR. Could you investigate the failure? https://github.com/trinodb/trino/actions/runs/10377395729/job/28731540853

[oskar-szwajkowski]

@oskar-szwajkowski TestIcebergGlueCatalogConnectorSmokeTest.testDeleteRowsConcurrently looks very flaky after this PR. Could you investigate the failure? https://github.com/trinodb/trino/actions/runs/10377395729/job/28731540853

Looking on it