feat(appmesh): add route retry policies #13353

misterjoshua · 2021-03-02T20:53:24Z

Adds route retry policies for http/http2 and gRPC routes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-03-02T20:53:27Z

misterjoshua · 2021-03-02T20:55:36Z

Apologies on the title of the commit 6c35500 - it should say feat(appmesh): add route retry policies, but I'm hesitant to force push now that I've already created the PR.

skinny85

Generally looks great @misterjoshua ! I have some strong opinions on the empty array validation, but not much comments beyond that.

packages/@aws-cdk/aws-appmesh/README.md

skinny85 · 2021-03-04T00:26:39Z

packages/@aws-cdk/aws-appmesh/README.md

+      grpcRetryEvents: [appmesh.GrpcRetryEvent.DEADLINE_EXCEEDED],
+      httpRetryEvents: [appmesh.HttpRetryEvent.CLIENT_ERROR],
+      tcpRetryEvents: [appmesh.TcpRetryEvent.CONNECTION_ERROR],


Same question here - can a GRPC Route retry on HTTP and TCP events?

@skinny85 Same as above for HTTP routes. gRPC is layered on HTTP/2 which is layered on TCP, so these routes can specify retry events for all the lower layers.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appmesh-route-grpcretrypolicy.html

https://docs.aws.amazon.com/app-mesh/latest/APIReference/API_GrpcRetryPolicy.html

skinny85 · 2021-03-04T00:28:21Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+  /**
+   * The timeout for each retry attempt
+   */
+  readonly perRetryTimeout: cdk.Duration;


How about just calling this retryTimeout?

I'd like that, but I do have a nit. The CDK design guidelines want us to use the terminology of the original service documentation. The CloudFormation calls it PerRetryTimeout (gRPC, HTTP). What do you think given this?

I think retryTimeout is clear enough to not cause any confusion here.

skinny85 · 2021-03-04T00:29:36Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+  /**
+   * Internal error
+   */
+  INTERNAL = 'internal',


Can you call this INTERNAL_ERROR then?

No strong objection here.

skinny85 · 2021-03-04T00:29:54Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+  INTERNAL = 'internal',
+
+  /**
+   * Resource was exhausted


What does this mean? I feel like more explanation is needed here.

I think the error aligns with gRPC code 8/RESOURCE_EXHAUSTED, which per gRPC documentation means, "Some resource has been exhausted, perhaps a per-user quota, or perhaps the entire file system is out of space." I'm not sure if I'm allowed to copy-paste that verbatim from the docs I just referenced - if I'm not, maybe we can say, "A resource has been exhausted." Thoughts?

How about adding that link to the gRPC documentation to the JSDocs of that field?

Ok no problem. I added @see links to each of the enum values.

skinny85 · 2021-03-04T00:35:16Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+   * permanently unavailable.
+   * @default - no retries for tcp events
+   */
+  readonly tcpRetryEvents?: TcpRetryEvent[];


The fact that either this, or httpRetryEvents is required, needs to be stated in the documentation of this property.

skinny85 · 2021-03-04T00:35:47Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+      if (props.retryPolicy.httpRetryEvents && props.retryPolicy.httpRetryEvents.length === 0) {
+        throw new Error('Specify at least one value in `httpRetryEvents` or leave it undefined');
+      }
+
+      if (props.retryPolicy.tcpRetryEvents && props.retryPolicy.tcpRetryEvents.length === 0) {
+        throw new Error('Specify at least one value in `tcpRetryEvents` or leave it undefined');
+      }


Remove this validation, and simply return undefined if a customer provided an empty list, in renderHttpRetryPolicy.

No reason to burden the customer with unnecessary errors if we know what to do.

No problemo.

skinny85 · 2021-03-04T00:36:34Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+      if (props.retryPolicy.grpcRetryEvents && props.retryPolicy.grpcRetryEvents.length === 0) {
+        throw new Error('Specify at least one value in `grpcRetryEvents` or leave it undefined');
+      }
+
+      if (props.retryPolicy.httpRetryEvents && props.retryPolicy.httpRetryEvents.length === 0) {
+        throw new Error('Specify at least one value in `httpRetryEvents` or leave it undefined');
+      }
+
+      if (props.retryPolicy.tcpRetryEvents && props.retryPolicy.tcpRetryEvents.length === 0) {
+        throw new Error('Specify at least one value in `tcpRetryEvents` or leave it undefined');
+      }


Same here (get rid of this validation, and return undefined for an empty list in renderGrpcRetryPolicy ).

Ok. No problem.

skinny85 · 2021-03-04T00:37:12Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+   * gRPC events on which to retry
+   * @default - no retries for gRPC events
+   */
+  readonly grpcRetryEvents?: GrpcRetryEvent[];


The fact that one of this, tcpRetryEvents or httpRetryEvents is required needs to be stated in the documentation of this property.

skinny85 · 2021-03-04T00:49:52Z

packages/@aws-cdk/aws-appmesh/test/test.route.ts

+      test.throws(() => {
+        appmesh.RouteSpec.http({
+          weightedTargets: [{ virtualNode }],
+          retryPolicy: {
+            httpRetryEvents: [],
+            tcpRetryEvents: [appmesh.TcpRetryEvent.CONNECTION_ERROR],
+            maxRetries: 5,
+            perRetryTimeout: cdk.Duration.seconds(10),
+          },
+        });
+      }, /at least one.*httpRetryEvents/);
+
+      test.throws(() => {
+        appmesh.RouteSpec.http({
+          weightedTargets: [{ virtualNode }],
+          retryPolicy: {
+            httpRetryEvents: [appmesh.HttpRetryEvent.CLIENT_ERROR],
+            tcpRetryEvents: [],
+            maxRetries: 5,
+            perRetryTimeout: cdk.Duration.seconds(10),
+          },
+        });
+      }, /at least one.*tcpRetryEvents/);
+
+      test.throws(() => {
+        appmesh.RouteSpec.grpc({
+          weightedTargets: [{ virtualNode }],
+          match: { serviceName: 'servicename' },
+          retryPolicy: {
+            grpcRetryEvents: [],
+            tcpRetryEvents: [appmesh.TcpRetryEvent.CONNECTION_ERROR],
+            maxRetries: 5,
+            perRetryTimeout: cdk.Duration.seconds(10),
+          },
+        });
+      }, /at least one.*grpcRetryEvents/i);
+
+      test.throws(() => {
+        appmesh.RouteSpec.grpc({
+          weightedTargets: [{ virtualNode }],
+          match: { serviceName: 'servicename' },
+          retryPolicy: {
+            httpRetryEvents: [],
+            tcpRetryEvents: [appmesh.TcpRetryEvent.CONNECTION_ERROR],
+            maxRetries: 5,
+            perRetryTimeout: cdk.Duration.seconds(10),
+          },
+        });
+      }, /at least one.*httpRetryEvents/i);
+
+      test.throws(() => {
+        appmesh.RouteSpec.grpc({
+          weightedTargets: [{ virtualNode }],
+          match: { serviceName: 'servicename' },
+          retryPolicy: {
+            httpRetryEvents: [appmesh.HttpRetryEvent.CLIENT_ERROR],
+            tcpRetryEvents: [],
+            maxRetries: 5,
+            perRetryTimeout: cdk.Duration.seconds(10),
+          },
+        });
+      }, /at least one.*tcpRetryEvents/i);


Like I commented on the production code, I think this is not good behavior.

I would remove all of these tests in favor of checking that providing an empty list results in the appropriate value not being written to the template. You can use the ABSENT constant from the assertion module to assert that the given value is not used - see example usage.

Cool. I didn't know this constant was a thing. Alright. This shouldn't be a problem.

misterjoshua · 2021-03-04T04:37:11Z

@skinny85 I saw your review comments. I'll look at working on this tomorrow when I'm at work. I think I have enough to go on to make some progress, but I'm hoping for some feedback on a few of the review comments. Thanks.

Pull request has been modified.

alexbrjo

It looks like you've addressed Adam's comments. Thanks for the PR, this LGTM!

alexbrjo · 2021-03-08T19:22:55Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

+      const tcpRetryEvents = props.retryPolicy.tcpRetryEvents ?? [];
+
+      if (grpcRetryEvents.length + httpRetryEvents.length + tcpRetryEvents.length === 0) {
+        throw new Error('You must specify one value for at least one of `grpcRetryEvents`, `httpRetryEvents`, `tcpRetryEvents`');


nit: You must specify one value for at least one of 'grpcRetryEvents', 'httpRetryEvents' or 'tcpRetryEvents'

Good catch! I've pushed up a fix for this.

packages/@aws-cdk/aws-appmesh/test/test.route.ts

skinny85

This is great @misterjoshua , thanks so much for the contribution!

mergify · 2021-03-11T00:57:39Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

misterjoshua · 2021-03-11T01:00:31Z

This is great @misterjoshua , thanks so much for the contribution!

My pleasure!

aws-cdk-automation · 2021-03-11T01:31:17Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: e683201
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

feat(ecs): add route retry policies

6c35500

misterjoshua changed the title ~~feat(ecs): add route retry policies~~ feat(appmesh): add route retry policies Mar 2, 2021

github-actions bot assigned skinny85 Mar 2, 2021

github-actions bot added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Mar 2, 2021

fix: cleaned out straggler from earlier refactoring

d567f3d

skinny85 previously requested changes Mar 4, 2021

View reviewed changes

misterjoshua added 5 commits March 4, 2021 09:01

docs: inline doc review changes

d1944aa

refactor: remove empty list validations and return undefined

c83c0f8

refactor: perRetryTimeout -> retryTimeout

a092ea1

refactor: INTERNAL -> INTERNAL_ERROR

c0e99f8

refactor: maxRetries -> retryAttempts

340d1c0

misterjoshua added 4 commits March 4, 2021 09:50

fix: typo for gRPC UNAVAILABLE error enum

e3cee3a

docs: update the readme example

9db78e2

docs: add a missing space before a @default

297ecaf

docs: more readme tweaks

30d433a

misterjoshua requested a review from skinny85 March 4, 2021 17:34

misterjoshua and others added 2 commits March 4, 2021 11:43

docs: add reference to gRPC docs for error enum

b1dd174

Merge branch 'master' into master

c1d82c1

alexbrjo approved these changes Mar 8, 2021

View reviewed changes

docs: fix typo in at-least-one error

c4993ac

skinny85 approved these changes Mar 11, 2021

View reviewed changes

Merge branch 'master' into master

e683201

misterjoshua mentioned this pull request Mar 11, 2021

feat(appmesh): add missing route match features #13350

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appmesh): add route retry policies #13353

feat(appmesh): add route retry policies #13353

misterjoshua commented Mar 2, 2021 •

edited

Loading

gitpod-io bot commented Mar 2, 2021 •

edited

Loading

misterjoshua commented Mar 2, 2021 •

edited

Loading

skinny85 left a comment

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021 •

edited

Loading

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021

skinny85 Mar 4, 2021

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021 •

edited

Loading

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021 •

edited

Loading

skinny85 Mar 4, 2021

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021 •

edited

Loading

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021

skinny85 Mar 4, 2021

skinny85 Mar 4, 2021

misterjoshua Mar 4, 2021

misterjoshua commented Mar 4, 2021 •

edited

Loading

alexbrjo left a comment

alexbrjo Mar 8, 2021

misterjoshua Mar 8, 2021

skinny85 left a comment

mergify bot commented Mar 11, 2021

misterjoshua commented Mar 11, 2021

aws-cdk-automation commented Mar 11, 2021

feat(appmesh): add route retry policies #13353

feat(appmesh): add route retry policies #13353

Conversation

misterjoshua commented Mar 2, 2021 • edited Loading

gitpod-io bot commented Mar 2, 2021 • edited Loading

misterjoshua commented Mar 2, 2021 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterjoshua Mar 4, 2021 • edited Loading

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

misterjoshua Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterjoshua Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterjoshua Mar 4, 2021 • edited Loading

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

misterjoshua commented Mar 4, 2021 • edited Loading

alexbrjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 11, 2021

misterjoshua commented Mar 11, 2021

aws-cdk-automation commented Mar 11, 2021

AWS CodeBuild CI Report

misterjoshua commented Mar 2, 2021 •

edited

Loading

gitpod-io bot commented Mar 2, 2021 •

edited

Loading

misterjoshua commented Mar 2, 2021 •

edited

Loading

misterjoshua Mar 4, 2021 •

edited

Loading

misterjoshua Mar 4, 2021 •

edited

Loading

misterjoshua Mar 4, 2021 •

edited

Loading

misterjoshua Mar 4, 2021 •

edited

Loading

misterjoshua commented Mar 4, 2021 •

edited

Loading