Do not retry iceberg operations on unrecoverable exceptions #19307

oskar-szwajkowski · 2023-10-07T21:08:14Z

Description

As in title

Additional context and related issues

Release notes

(X ) 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`)

electrum

We need a better way to do this than depending on a file system implementation class.

electrum · 2023-10-07T23:36:47Z

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java

@@ -237,7 +238,8 @@ protected void refreshFromMetadataLocation(String newLocation)
                            .withMaxRetries(20)
                            .withBackoff(100, 5000, MILLIS, 4.0)
                            .withMaxDuration(Duration.ofMinutes(10))
-                            .abortOn(failure -> failure instanceof ValidationException || isNotFoundException(failure))
+                            .abortOn(ValidationException.class, UnrecoverableS3OperationException.class)


This exception should be considered private to TrinoS3FileSystem (we should make it private) and won't be accessible after we complete the Hadoop removal project. Also, this is only for the legacy S3 file system.

should we have some more top level exception for filesystem errors that should not be retried? seems like common requirement, or maybe there is already exception that UnrecoverableS3OperationException could extend?

We could create a new exception for this, but we should first see if this is applicable to any of the new file systems. Can you investigate that?

I have introduced io.trino.filesystem.UnrecoverableTrinoFileSystemException, and make this private one extending from it

I can investigate at least some of them, I'll make a list of all implementors of TrinoFileSystem that I went / not went through, but let me create another issue/PR for it once this is merged and io.trino.filesystem.UnrecoverableTrinoFileSystemException is available

I want to make sure that this will be useful in the future and won't become dead code after we remove the legacy S3 file system.

...rino-filesystem/src/main/java/io/trino/filesystem/UnrecoverableTrinoFileSystemException.java

electrum · 2023-10-09T20:43:27Z

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java

@@ -237,7 +238,8 @@ protected void refreshFromMetadataLocation(String newLocation)
                            .withMaxRetries(20)
                            .withBackoff(100, 5000, MILLIS, 4.0)
                            .withMaxDuration(Duration.ofMinutes(10))
-                            .abortOn(failure -> failure instanceof ValidationException || isNotFoundException(failure))
+                            .abortOn(ValidationException.class, UnrecoverableS3OperationException.class)


I want to make sure that this will be useful in the future and won't become dead code after we remove the legacy S3 file system.

electrum

See comments

oskar-szwajkowski · 2023-10-12T11:32:06Z

@electrum could you take a look again?

github-actions · 2024-01-10T17:23:26Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-01-11T20:18:51Z

👋 @oskar-szwajkowski could you ensure any rebase necessary is done and CI passes. @electrum could you have a look again.

oskar-szwajkowski · 2024-01-18T14:01:11Z

👋 @oskar-szwajkowski could you ensure any rebase necessary is done and CI passes.

Rebased original branch, but there were no conflicts.

mosabua · 2024-01-18T20:54:23Z

@findepi and @electrum can you chime in here and figure out whats next?

findepi · 2024-01-31T15:24:40Z

@findepi and @electrum can you chime in here and figure out whats next?

since there is a red mark, a slight preference would be for David to follow-up

electrum · 2024-01-31T15:39:28Z

My comment about making this applicable to the new file systems has not been addressed. This feature is only useful for the deprecated S3 file system, so I'd rather not add it just for that.

mosabua · 2024-01-31T17:23:13Z

@oskar-szwajkowski could you address the request from @electrum please?

oskar-szwajkowski · 2024-02-01T13:44:26Z

@oskar-szwajkowski could you address the request from @electrum please?

@electrum I added handling of retryable / non retryable exceptions in new s3 based file system

github-actions · 2024-02-22T17:13:20Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-02-22T17:31:28Z

@electrum I think this is ready for another look from you.

github-actions · 2024-03-21T17:03:22Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

github-actions · 2024-04-11T17:43:38Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

mosabua · 2024-04-11T20:31:19Z

@oskar-szwajkowski @electrum @findepi @amogh-jahagirdar @bitsondatadev .. can you help out here to get this towards merge?

amogh-jahagirdar

I had some minor comments, but from an Iceberg connector perspective this seems good; definitley want to avoid wasting compute from retrying reading a file which will always fail.

amogh-jahagirdar · 2024-04-12T02:22:28Z

lib/trino-hdfs/src/main/java/io/trino/hdfs/s3/TrinoS3FileSystem.java

@@ -924,17 +925,12 @@ private static boolean isHadoopFolderMarker(S3ObjectSummary object)
        return object.getKey().endsWith("_$folder$");
    }

-    /**


I think I'd still leave the comment, that still applies no?

Oh I see you moved it to UnrecoverableIOException. Seems reasonable but I'd keep some of the examples like Forbidden in the comment in UnrecoverableIOException. That looks to be missing.

amogh-jahagirdar · 2024-04-12T02:24:57Z

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystem.java

@@ -255,6 +256,13 @@ public Optional<Location> createTemporaryDirectory(Location targetPath, String t
        return Optional.empty();
    }

+    private IOException asIOException(String message, SdkException exception)


Nit: I think this method can be static?

amogh-jahagirdar

One thing I'd suggest but I also understand if it's hard to do for this case, can we see about adding a test? This case seems tricky since we'd need to spoof bad auth or something in the smoke tests and then validate how many file IO interactions there were for metadata. Forcing a bad auth maybe hard given the current test setup but maybe I'm missing something. If there's a simple way to do that currently, I'd recommend that as well!

github-actions · 2024-05-10T17:40:36Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

github-actions · 2024-06-03T17:03:35Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

mosabua · 2024-06-03T18:33:51Z

Reopening to allow @electrum to review and chime in.

Convert terminal s3 filesystem exceptions to extend UnrecoverableIOException Convert SDKException to UnrecoverableIOException if they are not supposed to be retryable

oskar-szwajkowski · 2024-07-25T13:43:35Z

This PR is superseded by #22814

cla-bot bot added the cla-signed label Oct 7, 2023

github-actions bot added the iceberg Iceberg connector label Oct 7, 2023

electrum requested changes Oct 7, 2023

View reviewed changes

findepi approved these changes Oct 8, 2023

View reviewed changes

oskar-szwajkowski force-pushed the osz/do-not-rety-iceberg-on-terminal-exceptions branch 2 times, most recently from 6c03ad9 to 189dbfd Compare October 9, 2023 09:45

oskar-szwajkowski requested review from electrum and findepi October 9, 2023 09:47

github-actions bot added the tests:hive label Oct 9, 2023

electrum reviewed Oct 9, 2023

View reviewed changes

electrum requested changes Oct 9, 2023

View reviewed changes

oskar-szwajkowski force-pushed the osz/do-not-rety-iceberg-on-terminal-exceptions branch 2 times, most recently from 668130b to dcbae7c Compare October 10, 2023 08:58

oskar-szwajkowski requested a review from electrum October 10, 2023 09:00

github-actions bot added the stale label Jan 10, 2024

github-actions bot removed the stale label Jan 12, 2024

oskar-szwajkowski force-pushed the osz/do-not-rety-iceberg-on-terminal-exceptions branch from dcbae7c to 7440f5d Compare January 18, 2024 14:00

oskar-szwajkowski force-pushed the osz/do-not-rety-iceberg-on-terminal-exceptions branch from 7440f5d to 19071b1 Compare February 1, 2024 13:43

github-actions bot added the stale label Feb 22, 2024

github-actions bot removed the stale label Feb 28, 2024

github-actions bot added the stale label Mar 21, 2024

github-actions bot closed this Apr 11, 2024

mosabua reopened this Apr 11, 2024

amogh-jahagirdar approved these changes Apr 12, 2024

View reviewed changes

amogh-jahagirdar reviewed Apr 12, 2024

View reviewed changes

github-actions bot removed the stale label Apr 12, 2024

findepi removed the tests:hive label Apr 18, 2024

github-actions bot added the stale label May 10, 2024

github-actions bot closed this Jun 3, 2024

mosabua reopened this Jun 3, 2024

mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Jun 3, 2024

oskar-szwajkowski added 2 commits July 15, 2024 14:07

Introduce UnrecoverableIOException for terminal cases

57b7b03

Convert terminal s3 filesystem exceptions to extend UnrecoverableIOException Convert SDKException to UnrecoverableIOException if they are not supposed to be retryable

Do not retry iceberg operations on unrecoverable exceptions

6b44756

oskar-szwajkowski force-pushed the osz/do-not-rety-iceberg-on-terminal-exceptions branch from 19071b1 to 6b44756 Compare July 15, 2024 12:08

oskar-szwajkowski mentioned this pull request Jul 16, 2024

Retrying filesystem operations doesn't make sense for terminal exceptions (like bucket not exists) #22678

Closed

oskar-szwajkowski closed this Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not retry iceberg operations on unrecoverable exceptions #19307

Do not retry iceberg operations on unrecoverable exceptions #19307

oskar-szwajkowski commented Oct 7, 2023

electrum left a comment

electrum Oct 7, 2023

oskar-szwajkowski Oct 7, 2023

electrum Oct 8, 2023

oskar-szwajkowski Oct 9, 2023

electrum Oct 9, 2023

electrum Oct 9, 2023

electrum left a comment

oskar-szwajkowski commented Oct 12, 2023

github-actions bot commented Jan 10, 2024

mosabua commented Jan 11, 2024

oskar-szwajkowski commented Jan 18, 2024

mosabua commented Jan 18, 2024

findepi commented Jan 31, 2024

electrum commented Jan 31, 2024

mosabua commented Jan 31, 2024

oskar-szwajkowski commented Feb 1, 2024

github-actions bot commented Feb 22, 2024

mosabua commented Feb 22, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Apr 11, 2024

mosabua commented Apr 11, 2024

amogh-jahagirdar left a comment

amogh-jahagirdar Apr 12, 2024

amogh-jahagirdar Apr 12, 2024

amogh-jahagirdar Apr 12, 2024

amogh-jahagirdar left a comment

github-actions bot commented May 10, 2024

github-actions bot commented Jun 3, 2024

mosabua commented Jun 3, 2024

oskar-szwajkowski commented Jul 25, 2024

Do not retry iceberg operations on unrecoverable exceptions #19307

Do not retry iceberg operations on unrecoverable exceptions #19307

Conversation

oskar-szwajkowski commented Oct 7, 2023

Description

Additional context and related issues

Release notes

electrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

oskar-szwajkowski commented Oct 12, 2023

github-actions bot commented Jan 10, 2024

mosabua commented Jan 11, 2024

oskar-szwajkowski commented Jan 18, 2024

mosabua commented Jan 18, 2024

findepi commented Jan 31, 2024

electrum commented Jan 31, 2024

mosabua commented Jan 31, 2024

oskar-szwajkowski commented Feb 1, 2024

github-actions bot commented Feb 22, 2024

mosabua commented Feb 22, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Apr 11, 2024

mosabua commented Apr 11, 2024

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

github-actions bot commented May 10, 2024

github-actions bot commented Jun 3, 2024

mosabua commented Jun 3, 2024

oskar-szwajkowski commented Jul 25, 2024