error handling changes for Athena federation sdk #2278

Trianz-Akshay · 2024-09-19T12:32:31Z

Issue #, if available:

Description of changes:
Error handling improvement changes for Athena Federation SDK.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

burhan94 · 2025-01-23T15:28:01Z

...a-federation-sdk/src/main/java/com/amazonaws/athena/connector/lambda/QueryStatusChecker.java

@@ -126,7 +129,7 @@ private void checkStatus(String queryId, int attempt)
            if (e instanceof InvalidRequestException) {
                // query does not exist, so no need to keep calling Athena
                logger.debug("Athena reports query {} not found. Interrupting checker thread", queryId);
-                throw new InterruptedException();
+                throw new AthenaConnectorException(e, e.getMessage(), ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


This shouldn't be an internal service exception. This code path is best effort and is meant to kill the lambda early in the case that the calling Athena query is no longer running. We should keep this as InterruptedException since its just killing the thread. This doesn't get sent back to the caller.

reverted back to InterruptedException.

burhan94 · 2025-01-23T18:31:54Z

...eration-sdk/src/main/java/com/amazonaws/athena/connector/lambda/data/S3BlockSpillReader.java

@@ -75,7 +78,7 @@ public Block read(S3SpillLocation spillLocation, EncryptionKey key, Schema schem
            return block;
        }
        catch (IOException ex) {
-            throw new RuntimeException(ex);
+            throw new AthenaConnectorException(ex, ex.getMessage(), ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


This probably shouldn't be an internal service error. I'm wondering if we should introduce a RuntimeException to FederationSourceErrorCode.

took a look at the available exceptions from Glue. Lets keep this as a service error and we'll fix later

burhan94 · 2025-01-23T18:34:11Z

athena-federation-sdk/src/main/java/com/amazonaws/athena/connector/lambda/data/BlockUtils.java

-            throw new RuntimeException("block has " + block.getRowCount()
-                    + " rows but requested to check " + row);
+            throw new AthenaConnectorException("block has " + block.getRowCount()
+                    + " rows but requested to check " + row, ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


InvalidInputException

burhan94 · 2025-01-23T18:34:50Z

athena-federation-sdk/src/main/java/com/amazonaws/athena/connector/lambda/data/BlockUtils.java

                + field.getName() + " using value " + value
-                + " with minor type " + Types.getMinorTypeForArrowType(type), ex);
+                + " with minor type " + Types.getMinorTypeForArrowType(type), ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


FederationSourceErrorCode.INVALID_INPUT_EXCEPTION

burhan94 · 2025-01-23T18:35:16Z

athena-federation-sdk/src/main/java/com/amazonaws/athena/connector/lambda/data/BlockUtils.java

@@ -1015,13 +1018,13 @@ else if (value instanceof Boolean && (boolean) value) {
                    }
                    break;
                default:
-                    throw new IllegalArgumentException("Unknown type " + type);
+                    throw new AthenaConnectorException("Unknown type " + type, ErrorDetails.builder().errorCode(FederationSourceErrorCode.INVALID_INPUT_EXCEPTION.toString()).build());


OperationNotSupportedException

burhan94 · 2025-01-23T18:37:03Z

...-federation-sdk/src/main/java/com/amazonaws/athena/connector/lambda/data/S3BlockSpiller.java

-            throw new RuntimeException("Call generated more than " + maxRowsPerCall + "rows. Generating " +
-                    "too many rows per call to writeRows(...) can result in blocks that exceed the max size.");
+            throw new AthenaConnectorException("Call generated more than " + maxRowsPerCall + "rows. Generating " +
+                    "too many rows per call to writeRows(...) can result in blocks that exceed the max size.", ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


InvalidInputException

burhan94 · 2025-01-23T18:38:53Z

...-federation-sdk/src/main/java/com/amazonaws/athena/connector/lambda/data/S3BlockSpiller.java

@@ -512,7 +514,7 @@ private ThreadPoolExecutor makeAsyncSpillPool(SpillConfig config)
                }
                catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
-                    throw new RejectedExecutionException("Received an exception while submitting spillBlock task: ", e);
+                    throw new AthenaConnectorException("Received an exception while submitting spillBlock task: ", ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


This needs to stay as a RejectedExecutionException. The caller can apply a retry policy and the retry policy expects a RejectedExecutionException.

reverted back.

burhan94 · 2025-01-23T18:40:16Z

.../src/main/java/com/amazonaws/athena/connector/lambda/domain/spill/SpillLocationVerifier.java

@@ -93,7 +95,7 @@ void updateBucketState()
                state = BucketState.INVALID;
            }
            else {
-                throw new RuntimeException("Error while checking bucket ownership for " + bucket, ex);
+                throw new AthenaConnectorException(ex, "Error while checking bucket ownership for " + bucket, ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


AccessDeniedException

burhan94 · 2025-01-23T18:40:30Z

.../src/main/java/com/amazonaws/athena/connector/lambda/domain/spill/SpillLocationVerifier.java

            case INVALID:
-                throw new RuntimeException(String.format("spill_bucket: \"%s\" not found under your account. Please make sure you have access to the bucket and spill_bucket input has no trailing '/'", bucket));
+                throw new AthenaConnectorException(String.format("spill_bucket: \"%s\" not found under your account. Please make sure you have access to the bucket and spill_bucket input has no trailing '/'", bucket), ErrorDetails.builder().errorCode(FederationSourceErrorCode.INTERNAL_SERVICE_EXCEPTION.toString()).build());


InvalidInputException

Trianz-Akshay · 2025-01-27T12:36:40Z

Thank you for the review and suggestion. I have updated the code with the changes you recommended. However, I noticed that there are multiple exceptions applicable in certain scenarios. Earlier also we got different exceptions recommendations from other reviewers. If possible, could you provide a specific rule set or guidelines for handling these exceptions? This would help ensure consistency and correctness in the implementation.

Trianz-Akshay mentioned this pull request Sep 19, 2024

Feature/error handling #2271

Open

Trianz-Akshay mentioned this pull request Oct 16, 2024

error handling changes for Redshift #2275

Open

ritiktrianz force-pushed the feature/error_handling_athena_federation_sdk branch from f3cfdc5 to 623454c Compare November 8, 2024 08:20

error handling changes for sdk

2cf646f

ritiktrianz force-pushed the feature/error_handling_athena_federation_sdk branch from a4ac524 to 2cf646f Compare November 8, 2024 12:23

ritiktrianz added 4 commits November 11, 2024 12:49

Error handling changes for athena-federation-sdk

bd9d4eb

Checkstyle changes required for athena-federation-sdk

1648898

Checkstyle changes required for athena-federation-sdk

bc3102a

Checkstyle changes required for athena-federation-sdk

80fa8ec

yipez-spec approved these changes Nov 12, 2024

View reviewed changes

chngpe approved these changes Jan 22, 2025

View reviewed changes

macohen requested a review from burhan94 January 23, 2025 15:24

mschoeni1 approved these changes Jan 23, 2025

View reviewed changes

burhan94 requested changes Jan 23, 2025

View reviewed changes

Trianz-Akshay added 2 commits January 27, 2025 12:52

Merge branch 'master' into feature/error_handling_athena_federation_sdk

2dd0beb

review comment changes

18e6ace

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

error handling changes for Athena federation sdk #2278

error handling changes for Athena federation sdk #2278

Trianz-Akshay commented Sep 19, 2024

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

burhan94 Jan 23, 2025

Trianz-Akshay Jan 27, 2025

Trianz-Akshay commented Jan 27, 2025

error handling changes for Athena federation sdk #2278

Are you sure you want to change the base?

error handling changes for Athena federation sdk #2278

Conversation

Trianz-Akshay commented Sep 19, 2024

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

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

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

Trianz-Akshay commented Jan 27, 2025