feat(aws-dynamodb-global): support for global dynamodb tables #2082
Conversation
|
|
||
| Here is a minimal deployable Global DynamoDB tables definition: | ||
|
|
||
| ```javascript |
| super(scope, id, props); | ||
| const codeLocation = path.resolve(__dirname, "lambda", "handler.js.lambda"); | ||
| this.lambdaFunctionContent = fs.readFileSync(codeLocation, "utf8"); | ||
| this.lambdaFunction = new lambda.SingletonFunction(this, id + "-SingletonLambda", { |
There was a problem hiding this comment.
Why append to the id, you could just use SingletonLambda as the id.
There was a problem hiding this comment.
I found it easier to track the deployed stacks in CFN by having a common prefix in the stack ID. Otherwise, its difficult to keep track
| constructor(scope: cdk.Construct, id: string, props: DynamoDBGlobalStackProps) { | ||
| super(scope, id, props); | ||
| const codeLocation = path.resolve(__dirname, "lambda", "handler.js.lambda"); | ||
| this.lambdaFunctionContent = fs.readFileSync(codeLocation, "utf8"); |
There was a problem hiding this comment.
Use lambda.Code.asset(codeLocation) instead of reading yourself.
| .addAction("application-autoscaling:DeregisterScalableTarget") | ||
| .addAction("dynamodb:CreateGlobalTable") | ||
| .addAction("dynamodb:DescribeLimits") | ||
| .addAction("dynamodb:UpdateGlobalTable")); |
There was a problem hiding this comment.
The dynamodb:* permissions should be offered as static methods, either in this package or @aws-cdk/aws-dynamodb.
| .addAction("dynamodb:CreateGlobalTable") | ||
| .addAction("dynamodb:DescribeLimits") | ||
| .addAction("dynamodb:UpdateGlobalTable")); | ||
| this.customResource = new cfn.CustomResource(this, id + "-CfnCustomResource", { |
There was a problem hiding this comment.
Again, why append to the id?
| @@ -0,0 +1,81 @@ | |||
| { | |||
| "name": "@aws-cdk/aws-dynamodb-global", | |||
There was a problem hiding this comment.
Why do we need a new package as opposed to @aws-cdk/aws-dynamodb?
There was a problem hiding this comment.
Made more sense to me to have a new package since it has different requirements than the regular aws-dynamodb table - i.e. required tableName and streamSpecification. As well as its a higher level object than the dynamodb package.
Once CFN officially supports global-dynamodb tables, it would probably be easier to deprecate this package than remove it from an integrated portion of the aws-dynamodb package
There was a problem hiding this comment.
I think it's fine to have this as a separate module. Maybe we can add a comment in the dynamodb README that mentions it
| import globaldynamodb = require('@aws-cdk/aws-dynamodb-global'); | ||
| const CONSTRUCT_NAME = 'aws-cdk-dynamodb-global'; | ||
| const TABLE_PARTITION_KEY: Attribute = { name: 'hashKey', type: AttributeType.String }; | ||
| const STACK_PROPS: DynamoDBGlobalStackProps = { |
There was a problem hiding this comment.
This is not idiomatic, pass the properties directly when creating the construct:
new GlobalTable(stack, 'aws-cdk-dynamodb-global', {
dynamoProps: {
partitionKey: TABLE_PARTITION_KEY,
tableName: TABLE_NAME
},
regions: [ 'us-east-1', 'us-east-2', 'us-west-2' ],
// etc.
});| ``` | ||
|
|
||
| ## Implementation Notes | ||
| Global Dynamodb tables is an odd case currently. The way this package works - |
There was a problem hiding this comment.
Is there documentation on the recommended practice from AWS?
There was a problem hiding this comment.
AWS has no recommended practice for this since its not supported in CFN currently. This is the solution determined by myself and by chatting with CDK contributors
|
Changes have been posted to the PR, and responses to questions posted. |
|
|
||
| ```typescript | ||
| import globaldynamodb = require('@aws-cdk/aws-dynamodb-global'); | ||
| const CONSTRUCT_NAME = 'aws-cdk-dynamodb-global'; |
There was a problem hiding this comment.
These consts are not really adding much in terms of readability and they are only used once, just inline them
| ``` | ||
|
|
||
| ## Implementation Notes | ||
| Global Dynamodb tables is an odd case currently. The way this package works - |
There was a problem hiding this comment.
Make sure you capitalize service names properly and use the prefix "Amazon" or "AWS" (in this case it's Amazon DynamoDB).
|
|
||
| ### Notes | ||
|
|
||
| `DynamoDBGlobalStackProps.dynamoProps` is essentially a copy of `dynamodb.TableProps` - however `tableName` is *required*. |
There was a problem hiding this comment.
You can extract all the props besides tableName from TableProps into TableOptions interface and then have GlobaProps and TableProps inherit from Options (that's kind of how we do this)
There was a problem hiding this comment.
// in @aws-cdk/aws-dynamodb
export interface TableOptions {
// all props besides `tableName`
}
export interface TableProps extends TableOptions {
tableName?: string;
}
// in aws-dynamodb-global
export interface GlobalStackProps extends TableOptions {
tableName: string;
}|
|
||
| `DynamoDBGlobalStackProps.dynamoProps` is essentially a copy of `dynamodb.TableProps` - however `tableName` is *required*. | ||
|
|
||
| GlobalTable() will override any set `dynamoProps.streamSpecification` to be `NEW_AND_OLD_IMAGES` since this is a required attribute for global dynamodb tables to work. |
There was a problem hiding this comment.
Probably should error if users specified something else
|
More changes pushed. |
|
bump |
| * Properties for the mutliple DynamoDB tables to mash together into a | ||
| * global table | ||
| */ | ||
| export interface DynamoDBGlobalStackProps extends cdk.StackProps { |
| * Properties for the mutliple DynamoDB tables to mash together into a | ||
| * global table | ||
| */ | ||
| export interface GlobalTableProps extends cdk.StackProps { |
There was a problem hiding this comment.
Sorry, I probably didn't explain myself too well (interfaces can extend multiple bases):
export interface GlobalTableProps extends cdk.StackProps, dynamodb.TableOptions {
tableName: string;
regions: string[];
}Then, you don't need GlobalDynamoDBProps at all.
We like flat ;-)
| regions: string[]; | ||
| } | ||
|
|
||
| export class GlobalTable extends cdk.Construct { |
There was a problem hiding this comment.
Short description of this type (the README intro is great)
| @@ -0,0 +1,3 @@ | |||
| export * from "./aws-dynamodb-global"; | |||
| export * from "./lambda-global-dynamodb"; | |||
There was a problem hiding this comment.
Do we need to export this module? Seems like an internal implementation
| @@ -0,0 +1,81 @@ | |||
| { | |||
| "name": "@aws-cdk/aws-dynamodb-global", | |||
There was a problem hiding this comment.
I think it's fine to have this as a separate module. Maybe we can add a comment in the dynamodb README that mentions it
| const STACK_NAME = 'global-dynamodb-integ'; | ||
|
|
||
| // DynamoDB table parameters | ||
| const TABLE = 'GlobalTable'; |
There was a problem hiding this comment.
Why are these consts needed?
| /** | ||
| * Creates dynamoDB tables across regions that will be able to be globbed together into a global table | ||
| */ | ||
| public tables: MultiDynamoDBStack[]; |
There was a problem hiding this comment.
I think the API should only expose the Table objects and not the stack that wraps them. Furthermore, I don't even thing you need this MultiDynamoDBStack class (and of course you don't need it in the public API). You can just create those stacks in-place:
for (const reg of props.regions) {
const regionalStack = new cdk.Stack(this, '...', { env: { region: reg } });
const regionalTable = new dynamodb.Table(regionalStack, 'Table', props);
this.tables.push(regionalTable);
}This is how you should define the property:
public readonly tables = new Array<dynamodb.Table>();P.S. this is not ideal because the array is mutable. The "right" thing to do would be to return a clone:
private readonly _regionalTables = new Array<dynamodb.Table>();
public get regionalTables() {
return this._regionalTables.map(x => x);
}
There was a problem hiding this comment.
Chiming in—if mutability is a concern, would it make sense to use a library like https://github.com/immutable-js/immutable-js, or would that be too big of a dependency to expose as a public API in the CDK?
There was a problem hiding this comment.
We prefer to avoid taking such dependencies (and frankly JS/TS are sufficient). Soon Typescript will have x: readonly string[] and the world will be whole.
| export class GlobalTable extends cdk.Construct { | ||
|
|
||
| /** | ||
| * Creates dynamoDB tables across regions that will be able to be globbed together into a global table |
| }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please don't check in the handler.zip file! Where is the code and how is it bundled into a zip file?
There was a problem hiding this comment.
That's been addressed now
RomainMuller
added
feature-request
| * global table | ||
| */ | ||
| export interface RegionalTablesProps extends cdk.StackProps, dynamodb.TableOptions { | ||
| /** |
| */ | ||
| export interface RegionalTablesProps extends cdk.StackProps, dynamodb.TableOptions { | ||
| /** | ||
| * Enforces a particular table name to share across Global DynamoDB tables |
There was a problem hiding this comment.
Wording here feels off. "Enforces?" Ideally if we can copy & paste from official AWS docs that would be better. I would write something like "Name of the DynamoDB table to use across all regional tables. This is required for global tables".
| } | ||
|
|
||
| /** | ||
| * Global Tables builds upon DynamoDB’s global footprint to provide you with a fully managed, multi-region, |
There was a problem hiding this comment.
This describes Global Tables but the class is RegionalTables?
| * Properties for the mutliple DynamoDB tables to mash together into a | ||
| * global table | ||
| */ | ||
| export interface RegionalTablesProps extends cdk.StackProps, dynamodb.TableOptions { |
There was a problem hiding this comment.
Why is this called "RegionalTables" instead of "GlobalTable"?
There was a problem hiding this comment.
You asked for the class to be renamed?
#2082 (comment)
There was a problem hiding this comment.
Not the class, the property name. It's: GlobalTable.regionalTables. Otherwise it doesn't make sense...
| constructor(scope: cdk.Construct, id: string, props: RegionalTablesProps) { | ||
| super(scope, id); | ||
| this._regionalTables = []; | ||
| if ( props.streamSpecification != null && props.streamSpecification !== dynamodb.StreamViewType.NewAndOldImages) { |
| } else if (event.RequestType === "Update") { | ||
| console.log("UDPATE!"); | ||
| // Put your custom update logic here | ||
| sendResponse(event, context, "SUCCESS", { Message: "Resource update successful!" }); |
There was a problem hiding this comment.
Does it makes sense that update is a no-op? What if I add a region for example?
| } else { | ||
| console.log("FAILED!"); | ||
| sendResponse(event, context, "FAILED"); | ||
| } |
There was a problem hiding this comment.
You must catch all exceptions and send a FAILED response. Otherwise, any exception will cause the CFN deployment to timeout after 4 hours.
Then, all failure cases should just "throw".
Ideally, if you can also have only a single point in your code that sends a SUCCESS response, it would be safer, especially if someone updates this code in the future and forgets to handle some case (alternatively, "throw" when you fall through).
| } | ||
| }; | ||
|
|
||
| function setupWatchdogTimer(event, context, callback) { |
| context.done(); | ||
| }); | ||
|
|
||
| // write data to request body |
There was a problem hiding this comment.
Please add a unit test for this handler
| @@ -0,0 +1,3 @@ | |||
| 'use strict'; | |||
|
|
|||
| const handler = require('../lib/lambda/handler'); | |||
There was a problem hiding this comment.
Not enough.. You'll have to mock stuff..
There was a problem hiding this comment.
lol I know, just checked in what I had gotten done so far this morning :)
|
@KingOfPoptart could you please rebase this and we'll merge that in |
KingOfPoptart
requested review from
RomainMuller,
skinny85 and
SoManyHs
as code owners
|
This PR has been replaced by #2251 |
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.