New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: first commit of globaltable l2 #25091

Closed

pattasai wants to merge 2 commits into main from ddb-GlobalTable-l2

Contributor

pattasai commented Apr 12, 2023 •

edited by iliapolo

Loading

This PR creates L2 construct for globalTable of Dynamodb

Resolves #16118

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


          feat: first commit of globaltable l2

e6fb0fc

pattasai added feature-request @aws-cdk/aws-dynamodb labels

pattasai self-assigned this

github-actions bot added the p2 label

aws-cdk-automation requested a review from a team

April 12, 2023 19:14

pattasai requested review from rix0rrr and removed request for a team

April 12, 2023 19:15

mergify bot added the contribution/core label

aws-cdk-automation requested changes

View reviewed changes

Collaborator

aws-cdk-automation left a comment

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

rix0rrr reviewed

View reviewed changes

Contributor

rix0rrr left a comment

I like where this is going!

I'm noticing a whole bunch of code that is either no longer being used, or that should now be shared between table.ts and global-table.ts, so it's kinda hard to review on its own.

I assume you'll be doing a cleanup at some point, right?

packages/aws-cdk-lib/aws-dynamodb/lib/global-table.ts Outdated

Comment on lines 23 to 38

+              // https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Limits.html#limits-secondary-indexes
+              const MAX_LOCAL_SECONDARY_INDEX_COUNT = 5;
+              /**
+               * Options for configuring a system errors metric that considers multiple operations.
+               */
+              export interface SystemErrorsForOperationsMetricOptions extends cloudwatch.MetricOptions {
+                  /**
+                   * The operations to apply the metric to.
+                   *
+                   * @default - All operations available by DynamoDB tables will be considered.
+                   */
+                  readonly operations?: Operation[];
+              }

Contributor

rix0rrr Apr 14, 2023

I appreciate that you copy/pasted all of this out to get started.

Now that we're working on landing this as "official" code in the CDK repository, it makes sense to start cleanup.

For all the definitions that are shared between table.ts and global-table.ts, it makes sense to move them out to a separate file (call it shared.ts for example), and start referencing them from there.

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

+              /**
+               * Represents the table schema attributes.
+               */
+              export interface SchemaOptions {

Contributor

rix0rrr Apr 14, 2023

I don't like this type. Remember how the existence of this type, shared as it was everywhere, made it impossible for us to make a change we needed to make to address one issue? We talked about this.

I hope you're not using it below :). If you are, just copy/paste these two fields into where you were using it before.

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

Comment on lines +1329 to +1332

+                      if (props.replicas) {
+                          if (props.stream && props.stream !== StreamViewType.NEW_AND_OLD_IMAGES) {
+                              throw new Error('`stream` must be set to `NEW_AND_OLD_IMAGES` when specifying `replicationRegions`');
+                          }

Contributor

rix0rrr Apr 14, 2023

This is true. Do we also check this when addReplica is called?

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

+                          this.billingMode = props.billingMode ?? BillingMode.PAY_PER_REQUEST;
+                      } else {
+                          this.billingMode = props.billingMode ?? BillingMode.PAY_PER_REQUEST;

Contributor

rix0rrr Apr 14, 2023

This line occurs twice. It could occur only once, and doesn't have to be inside the if (props.replicas) check, right?

packages/aws-cdk-lib/aws-dynamodb/lib/global-table.ts Outdated

+                   *
+                   * @default - no replica tables are created
+                   */
+                  readonly replicationRegions?: string[];

Contributor

rix0rrr Apr 14, 2023

Shouldn't there be properties associated with replicas?

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

+                   * When an item in the table is modified, StreamViewType determines what information
+                   * is written to the stream for this table.
+                   *
+                   * @default - streams are disabled unless `replicationRegions` is specified

Contributor

rix0rrr Apr 14, 2023

👍

packages/aws-cdk-lib/aws-dynamodb/lib/global-table.ts Outdated

Comment on lines 273 to 287

+                  /**
+                   * Indicates whether CloudFormation stack waits for replication to finish.
+                   * If set to false, the CloudFormation resource will mark the resource as
+                   * created and replication will be completed asynchronously. This property is
+                   * ignored if replicationRegions property is not set.
+                   *
+                   * DO NOT UNSET this property if adding/removing multiple replicationRegions
+                   * in one deployment, as CloudFormation only supports one region replication
+                   * at a time. CDK overcomes this limitation by waiting for replication to
+                   * finish before starting new replicationRegion.
+                   *
+                   * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-dynamodb-globaltable.html#cfn-dynamodb-globaltable-replicas
+                   * @default true
+                   */
+                  readonly waitForReplicationToFinish?: boolean;

Contributor

rix0rrr Apr 14, 2023

This seems like a holdover from when we used to do the replication ourselves. Is this still necessary?

packages/aws-cdk-lib/aws-dynamodb/lib/global-table.ts Outdated

+                   *
+                   * @param regions regions where to create tables
+                   */
+                  private createReplicaTables(regions: string[], timeout?: Duration, waitForReplicationToFinish?: boolean) {

Contributor

rix0rrr Apr 14, 2023

This whole function needs to disappear, doesn't it?


          feat: shared environment support added

5ce4e55

Collaborator

aws-cdk-automation commented Apr 18, 2023

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5ce4e55
Result: FAILED
Build Logs (available for 30 days)

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

github-actions bot added effort/large p1 and removed p2 labels

Collaborator

aws-cdk-automation commented May 11, 2023

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to a test file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

mrgrain marked this pull request as draft

July 6, 2023 13:15

Contributor

mrgrain commented Jul 6, 2023

Drafting this until someone can work on it again.

Contributor

kaizencc commented Jul 31, 2023

Superseded by #26563

kaizencc closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

@aws-cdk/aws-dynamodb contribution/core effort/large feature-request p1