feat(dynamodb): add option to skip waiting for global replication to finish by TheSPD · Pull Request #16983 · aws/aws-cdk

TheSPD · 2021-10-14T17:53:28Z

Motivation - On large tables, replication takes long time to complete. CloudFormation has a hard timeout of 1 hour on the Custom Resources, to bypass this, we want to have the replication continue in background based on a property.

Fixes #16611

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

gitpod-io · 2021-10-14T17:53:31Z

mergify · 2021-10-14T17:54:03Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

skinny85

Thanks for the contribution @TheSPD! Looks fine overall, I have a couple of minor comments though.

skinny85 · 2021-10-14T18:47:17Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+   * Whether Asynchronous Replication is enabled. If enabled, the
+   * CloudFormation resource will not wait for replication operation to be
+   * completed.
+   * DO NOT USE this for adding multiple replication regions in one deployment,
+   * as CloudFormation only supports one region replication at a time.


These docs need some work I think.

The first sentence doesn't really say anything.

It doesn't mention that it's related to the replicationRegions property, and is ignored if that property is not set.

Not sure I understand what you mean by the warning...? Each region gets its own Custom Resource, so what's the problem in enabling it with multiple regions?

Agree on 1 and 2 will add that;

On 3
I'm referring to this.

the current code bypasses this by adding some CFnCondition magic over the replicas(if multiple).

Ok, let's reword that section a little bit then, and link to that document.

skinny85 · 2021-10-14T18:47:52Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

+   *
+   * @default false
+   */
+  readonly asynchronousReplicationEnabled?: boolean;


I'm not sure I love the name. How about waitForReplicationToFinish?: boolean, default true?

skinny85 · 2021-10-14T18:51:27Z

packages/@aws-cdk/aws-dynamodb/lib/replica-handler/index.ts

  const replicas = data.Table?.Replicas ?? [];
  const regionReplica = replicas.find(r => r.RegionName === event.ResourceProperties.Region);
  const replicaActive = !!(regionReplica?.ReplicaStatus === 'ACTIVE');
+  const asynchronousReplicationEnabled = event.ResourceProperties.AsynchronousReplicationEnabled;


Let's turn this into a negative, so that it's not enabled by default. So, let's call the property something like SkipReplicationCompletedWait, and it will be false if undefined. Even better, let's make the false case explicit:

Suggested change

const asynchronousReplicationEnabled = event.ResourceProperties.AsynchronousReplicationEnabled;

const skipReplicationCompletedWait = event.ResourceProperties.SkipReplicationCompletedWait ?? false;

skinny85 · 2021-10-14T18:56:42Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

        properties: {
          TableName: this.tableName,
          Region: region,
+          AsynchronousReplicationEnabled: asynchronousReplicationEnabled || false,


Let's not do any defaulting here. This way, the existing templates will be unchanged.

Suggested change

AsynchronousReplicationEnabled: asynchronousReplicationEnabled || false,

SkipReplicationCompletedWait: waitForReplicationToFinish === undefined ? undefined : !waitForReplicationToFinish,

skinny85 · 2021-10-14T18:57:55Z

packages/@aws-cdk/aws-dynamodb/test/dynamodb.test.ts

+        AsynchronousReplicationEnabled: false,
      },
      Condition: 'TableStackRegionNotEqualseucentral199D46FC0',
    }, ResourcePart.CompleteDefinition);


We need a new test here, passing waitForReplicationToFinish as false.

skinny85 · 2021-10-14T18:58:14Z

packages/@aws-cdk/aws-dynamodb/test/integ.global.expected.json

        "TableName": {
          "Ref": "TableCD117FA1"
        },
-        "Region": "eu-west-2"


If you follow my advice from above, all of these changes can be reverted.

Pull request has been modified.

packages/@aws-cdk/aws-dynamodb/lib/table.ts

skinny85

Looks great, thanks for the contribution @TheSPD!

mergify · 2021-10-15T19:43:52Z

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).

aws-cdk-automation · 2021-10-15T20:35:44Z

AWS CodeBuild CI Report

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

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

mergify · 2021-10-15T20:36:14Z

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).

Our CFN Custom Resource for global DynamoDB Tables did not correctly handle changing the `waitForReplicationToFinish` parameter introduced in aws#16983 - adding it to an existing replica would error out by attempting to re-create the existing replica. Fix this by adding a check in the Custom Resource whether a replica already exists.

…#17842) Our CFN Custom Resource for global DynamoDB Tables did not correctly handle changing the `waitForReplicationToFinish` parameter introduced in #16983 - adding it to an existing replica would error out by attempting to re-create the existing replica. Fix this by adding a check in the Custom Resource whether a replica already exists. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…finish (aws#16983) Motivation - On large tables, replication takes long time to complete. CloudFormation has a hard timeout of 1 hour on the Custom Resources, to bypass this, we want to have the replication continue in background based on a property. Fixes aws#16611 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…aws#17842) Our CFN Custom Resource for global DynamoDB Tables did not correctly handle changing the `waitForReplicationToFinish` parameter introduced in aws#16983 - adding it to an existing replica would error out by attempting to re-create the existing replica. Fix this by adding a check in the Custom Resource whether a replica already exists. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

TheSPD changed the title ~~feature(aws-dynamodb): delegate replica creation to background~~ feat(aws-dynamodb): delegate replica creation to background Oct 14, 2021

TheSPD force-pushed the feature-dynamodb-background-replica branch from 98c73b8 to ca08007 Compare October 14, 2021 18:20

skinny85 previously requested changes Oct 14, 2021

View reviewed changes

skinny85 changed the title ~~feat(aws-dynamodb): delegate replica creation to background~~ feat(dynamodb): add option to skip waiting for global replication to finish Oct 14, 2021

TheSPD force-pushed the feature-dynamodb-background-replica branch from ca08007 to 436fd2a Compare October 14, 2021 22:09

TheSPD force-pushed the feature-dynamodb-background-replica branch 2 times, most recently from d976eb4 to 4f6e696 Compare October 14, 2021 22:36

feature(aws-dynamodb): delegate replica creation to background

6633c39

TheSPD force-pushed the feature-dynamodb-background-replica branch from 4f6e696 to 6633c39 Compare October 14, 2021 22:45

TheSPD requested a review from skinny85 October 15, 2021 00:02

skinny85 reviewed Oct 15, 2021

View reviewed changes

packages/@aws-cdk/aws-dynamodb/lib/table.ts Outdated Show resolved Hide resolved

Remove <a> from @see

4da31d4

skinny85 approved these changes Oct 15, 2021

View reviewed changes

skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Oct 15, 2021

Merge branch 'master' into feature-dynamodb-background-replica

26387ea

mergify bot merged commit 254601f into aws:master Oct 15, 2021

skinny85 mentioned this pull request Dec 4, 2021

fix(dynamodb): changing waitForReplicationToFinish fails deployment #17842

Merged

	const asynchronousReplicationEnabled = event.ResourceProperties.AsynchronousReplicationEnabled;
	const skipReplicationCompletedWait = event.ResourceProperties.SkipReplicationCompletedWait ?? false;

	AsynchronousReplicationEnabled: asynchronousReplicationEnabled \|\| false,
	SkipReplicationCompletedWait: waitForReplicationToFinish === undefined ? undefined : !waitForReplicationToFinish,

Conversation

TheSPD commented Oct 14, 2021 • edited by skinny85 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Oct 14, 2021

Uh oh!

mergify bot commented Oct 14, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Oct 15, 2021

Uh oh!

aws-cdk-automation commented Oct 15, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheSPD commented Oct 14, 2021 •

edited by skinny85

Loading