Updating SDK exceptions #301

spfink · 2017-11-22T18:03:15Z

Updates the exceptions used by the SDK. Lots of files here but mostly just updating to use the new exceptions. Part 1 of additional changes around exceptions.

Description

Removes AmazonClientException, AmazonServiceException, and SdkBaseException.

Adds SdkClientException, SdkServiceException and SdkException.

SdkClientException extends SdkException
SdkServiceException extends SdkException

SdkException has a retryable() method that will allow generated exceptions to override and set this value.

Also adds NonRetryableException and RetryableException which are meant to be used by customers to explicitly mark exceptions in interceptors or lambdas.

Testing

All unit tests pass and have been updated as needed.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed
A short description of the change has been added to the CHANGELOG

License

I confirm that this pull request can be released under the Apache 2 license

zoewangg · 2017-12-04T22:22:41Z

core/src/main/java/software/amazon/awssdk/core/http/DefaultErrorResponseHandler.java

    }

-    private AmazonServiceException createAse(HttpResponse errorResponse) throws Exception {
+    private SdkServiceException createAse(HttpResponse errorResponse) throws Exception {


might want to rename this method since it's not returning AmazonServiceException anymore. createServiceException?

Or createSse, that won't be ambiguous at all ;)

Ah, yep, will update. Thought I had most of them but found a new set. Hard to find all the cases of "ase" and all of it's casing throughout the code.

zoewangg · 2017-12-04T22:24:01Z

core/src/main/java/software/amazon/awssdk/core/http/DefaultErrorResponseHandler.java

+        for (Unmarshaller<SdkServiceException, Node> unmarshaller : unmarshallerList) {
+            SdkServiceException exception = unmarshaller.unmarshall(document);
+            if (exception != null) {
+                exception.statusCode(errorResponse.getStatusCode());


could errorResponse be null?

I'm not currently aware of a real case where it could be null.

it's not massively expensive nor that big a maintenance burden - prolly just leave it in for now.

zoewangg · 2017-12-04T22:35:41Z

core/src/main/java/software/amazon/awssdk/core/exception/SdkServiceException.java

+    }
+
+    public void headers(Map<String, String> headers) {
+        this.headers = headers;


should we use unmodifiableMap here?

Yes, will update

varunnvs92 · 2017-12-11T21:46:00Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/specs/Ec2ProtocolSpec.java

-                .beginControlFlow("catch (AmazonServiceException ase)")
-                .beginControlFlow("if (ase.getErrorCode().equals($S) && ase.getStatusCode() == 412)",
+                .beginControlFlow("catch (SdkServiceException exception)")
+                .beginControlFlow("if (exception.errorCode().equals($S) && ase.statusCode() == 412)",


ase.statusCode() -> exception.statusCode()
ase has been renamed to exception.

Similarly in line 76

varunnvs92 · 2017-12-11T21:47:30Z

...rc/main/java/software/amazon/awssdk/codegen/protocol/ApiGatewayProtocolMetadataProvider.java

@@ -15,7 +15,7 @@

 package software.amazon.awssdk.codegen.protocol;

-import software.amazon.awssdk.core.SdkBaseException;
+import software.amazon.awssdk.core.exception.SdkServiceException;


Should it be SdkException?

Nope. The base exception for service exceptions is SdkServiceException

varunnvs92 · 2017-12-11T21:52:25Z

core/src/main/java/software/amazon/awssdk/core/exception/AbortedException.java

-    public AbortedException() {
-        super("");
+    public AbortedException(Throwable cause) {
+        super(null, cause);


why null here while line 31 uses "Aborted"?

I can add it back in but this is what the prior defaults were.

Nothing -> just message "Aborted."

Message -> just message

Cause -> Just cause (message = null)

varunnvs92 · 2017-12-11T21:54:46Z

core/src/main/java/software/amazon/awssdk/core/exception/AbortedException.java

    }

    /**
     * {@inheritDoc}
     * An aborted exception is not intended to be retried.
     */
    @Override
-    public boolean isRetryable() {
+    public boolean retryable() {


Don't think we need this method here. SdkException by default returns false.

varunnvs92 · 2017-12-11T21:56:19Z

core/src/main/java/software/amazon/awssdk/core/exception/ResetException.java

@@ -30,7 +40,7 @@ public ResetException(String message, Throwable t) {
     * A stream reset exception cannot be retried.
     */
    @Override
-    public boolean isRetryable() {
+    public boolean retryable() {


same as above. can be removed.

varunnvs92 · 2017-12-11T22:09:08Z

core/src/main/java/software/amazon/awssdk/core/runtime/transform/StandardErrorUnmarshaller.java

-            ase.setErrorType(AmazonServiceException.ErrorType.Service);
-        } else if (errorType.equalsIgnoreCase("Sender")) {
-            ase.setErrorType(AmazonServiceException.ErrorType.Client);
+        if ("Receiver".equals(errorType)) {


where do the values "Receiver" and "Sender"? Also document it

I'll add documentation.

zoewangg · 2017-12-12T21:51:45Z

core/src/main/java/software/amazon/awssdk/core/http/pipeline/stages/HandleResponseStage.java

@@ -89,6 +89,10 @@ private OutputT handleSuccessResponse(HttpResponse httpResponse, RequestExecutio
        } catch (IOException | InterruptedException | RetryableException e) {
            throw e;
        } catch (Exception e) {
+            if (e instanceof SdkException && ((SdkException) e).retryable()) {


could successResponseHandler.handle throw SdkException? I looked at some implementations but didn't find any.

It could with S3.

Maybe when using lambdas as well but not sure on that.

zoewangg · 2017-12-12T22:10:03Z

...rc/main/java/software/amazon/awssdk/core/internal/http/response/AwsErrorResponseHandler.java

-                exception.setStatusCode(statusCode);
-                exception.setErrorType(AmazonServiceException.ErrorType.Client);
-                exception.setErrorCode("Request entity too large");
+                SdkServiceException exception = new SdkServiceException("Request entity too large");


should this be a SdkClientException since it's a client error? would it retry if we use SdkServiceException here?

Not really. We still pull out the status code and status text associated with the response from the service. The SdkClientException doesn't have these fields. It also doesn't portray as well that the request was made and the service rejected it as the clients fault.

I'm actually thinking this might be dead code. The error response handler should always be able to throw an unmarshalled exception no matter what happens.

zoewangg · 2017-12-12T23:20:33Z

LGTM! don't forget to add changelog and good luck with merging conflicts..🙂

shorea · 2017-12-14T00:37:55Z

Here's my general customer focused feedback, haven't looked at the code yet.

We should reduce the name clashes in the base exception type. We are reserving a lot of top level names that could clash with modeled fields. One example is errorCode which clashes with Inspector and we have to customize around it. Can we push some of these down to sub-container objects to make this less of a problem.
Error code is definitely AWS specific and not something we should have in the base service exception class. Request ID is questionable so but it's generic enough that it could apply more broadly. I'm thinking we should have another AwsServiceException layer for things that only apply to AWS services. The current SdkServiceException can then be focused on the wire exception rather than make any kinds of protocol assumptions like error code, error type,etc.
Exceptions are very mutable. This is inconsistent with the generated classes which are immutable in respect to their properties. Any plans to address that? Also there are toBuilder and copy methods that just don't preserve the top level fields.
Headers are a map of string to string which is inconsitent with how we represent them in the SdkHttpResponse. To alleviate the name clashes and make our lives easier should we just provide access to the SdkHttpResponse object which has status code/status text, headers? We'd need to provide content separately since it's buffered
Have we thought about exception serialization at all? How does that work? The base classes don't have traditional getters, the builders for the generated classes have getters but don't extend from any common base.
The retryable method might be confusing for customers if we aren't overriding it in actual retryable exceptions. Can we hide this for now until it's more widely used in modeled exceptions?
Should we be returning Optional in some of the exception getters since they won't always be present?

shorea · 2017-12-14T00:53:21Z

core/src/main/java/software/amazon/awssdk/core/exception/Crc32MismatchException.java

 */
-public class Crc32MismatchException extends IOException {
+@SdkPublicApi
+public class Crc32MismatchException extends SdkClientException {


Questioning why we don't retry this. Seems like a good candidate.

shorea · 2017-12-14T00:54:46Z

core/src/main/java/software/amazon/awssdk/core/exception/NonRetryableException.java


 /**
- * Exception that is NOT retried in default retry policies.
+ * Extension of {@link SdkException} that can be used by clients to
+ * explicitly have an exception not retried. This exception will never be


Last sentence here confuses me.

shorea · 2017-12-14T00:59:24Z

core/src/main/java/software/amazon/awssdk/core/http/pipeline/stages/RetryableStage.java

@@ -107,11 +107,11 @@ private RetryExecutor(SdkHttpFullRequest request, RequestExecutionContext contex
                    } else {
                        retryHandler.setLastRetriedException(handleUnmarshalledException(response));
                    }
-                } catch (AmazonServiceException e) {
+                } catch (SdkServiceException e) {
                    // TODO This can be cleaned up a bit if we have separate hierarchies for service and client exceptions


Can we address this now?

shorea · 2017-12-14T01:01:36Z

...rc/main/java/software/amazon/awssdk/core/internal/http/response/AwsErrorResponseHandler.java

-        return ase;
+    public SdkServiceException handle(HttpResponse response,
+                                      ExecutionAttributes executionAttributes) throws Exception {
+        final SdkServiceException exception = (SdkServiceException) handleServiceException(response, executionAttributes);


Is there something we can do about the case here? If not it's not a big deal.

shorea · 2017-12-14T01:02:34Z

...rc/main/java/software/amazon/awssdk/core/internal/http/response/AwsErrorResponseHandler.java

-                exception.setStatusCode(statusCode);
-                exception.setErrorType(AmazonServiceException.ErrorType.Client);
-                exception.setErrorCode("Request entity too large");
+                SdkServiceException exception = new SdkServiceException("Request entity too large");


I'm actually thinking this might be dead code. The error response handler should always be able to throw an unmarshalled exception no matter what happens.

shorea · 2017-12-14T01:02:46Z

...rc/main/java/software/amazon/awssdk/core/internal/http/response/AwsErrorResponseHandler.java

-                exception.setStatusCode(statusCode);
-                exception.setErrorType(AmazonServiceException.ErrorType.Service);
-                exception.setErrorCode(response.getStatusText());
+                SdkServiceException exception = new SdkServiceException(response.getStatusText());


Same here I think this is dead code.

shorea · 2017-12-14T01:05:39Z

core/src/main/java/software/amazon/awssdk/core/retry/RetryUtils.java

    }

    /**
-     * @deprecated In favor of {@link RetryUtils#isThrottlingException(SdkBaseException)}
+     * @deprecated In favor of {@link RetryUtils#isThrottlingException(SdkException)}
     */
    @Deprecated


Can we handle some of these deprecations now?

shorea · 2017-12-14T01:08:34Z

core/src/main/java/software/amazon/awssdk/core/runtime/transform/StandardErrorUnmarshaller.java

+            return ErrorType.CLIENT;
+        } else {
+            return ErrorType.fromValue(errorType);
+        }
    }

    @ReviewBeforeRelease("We shouldn't have S3 speific code in core. Also the way this is doesn't" +


Do we still need this with Zoe's change? Or has that not been merged yet?

it got removed in my PR, which got merged after this. so that's old code you are seeing 🙂

zoewangg reviewed Dec 4, 2017

View reviewed changes

varunnvs92 reviewed Dec 11, 2017

View reviewed changes

varunnvs92 suggested changes Dec 11, 2017

View reviewed changes

spfink force-pushed the finks/exceptions branch from bd6510d to d17a518 Compare December 12, 2017 18:00

zoewangg reviewed Dec 12, 2017

View reviewed changes

varunnvs92 approved these changes Dec 12, 2017

View reviewed changes

Refactoring exceptions used by the SDK.

d78ebce

spfink force-pushed the finks/exceptions branch from d17a518 to d78ebce Compare December 12, 2017 23:52

spfink merged commit c49d908 into master Dec 13, 2017

shorea reviewed Dec 14, 2017

View reviewed changes

spfink mentioned this pull request Aug 8, 2018

Refactor: Exceptions #22

Closed

shorea deleted the finks/exceptions branch November 23, 2018 03:53

Updating SDK exceptions #301

Updating SDK exceptions #301

Conversation

spfink commented Nov 22, 2017 • edited Loading

Description

Testing

Types of changes

Checklist

License

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

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

zoewangg commented Dec 12, 2017

shorea commented Dec 14, 2017

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

spfink commented Nov 22, 2017 •

edited

Loading