Skip to content
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.

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

Support Versioned and DeltaSyncConfig on DynamoDB DataSources #313

Closed

Conversation

@yyoshiki41
Copy link

yyoshiki41 commented Mar 14, 2020

AppSync currently supports versioning on DynamoDB data sources.
So, I added this options and updated the example of serverless.yml on README.

fixed #309


Example of CloudFormation template is below.

  DynamoDBPostsTableDatasource:
    Type: AWS::AppSync::DataSource
    Properties:
      Type: AMAZON_DYNAMODB
      Name: posts
      ApiId:
        Fn::GetAtt:
        - DeltaSyncApi
        - ApiId
      ServiceRoleArn: !GetAtt AppSyncTutorialAmazonDynamoDBRole.Arn
      DynamoDBConfig:
        TableName:
          Ref: DynamoDBTableDeltaSyncPostsTable
        AwsRegion:
          Ref: AWS::Region
        UseCallerCredentials: FALSE
        Versioned: TRUE
        DeltaSyncConfig:
          DeltaSyncTableName:
            Ref: DynamoDBTableDeltaSyncDeltaTable
          DeltaSyncTableTTL: 60
          BaseTableTTL: 0

Thanks!


This change is Reviewable

Copy link
Collaborator

bboure left a comment

Thank you @yyoshiki41 for your contrib.
I am not familiar with versioned options yet. But this LGTM.

return {
BaseTableTTL: config.deltaSyncConfig.baseTableTTL,
DeltaSyncTableName: config.deltaSyncConfig.deltaSyncTableName,
DeltaSyncTableTTL: config.deltaSyncConfig.deltaSyncTableTTL,

This comment has been minimized.

Copy link
@bboure

bboure Mar 14, 2020

Collaborator

@yyoshiki41 what would happen if one of baseTableTTL, deltaSyncTableName or deltaSyncTableTTL is undefined?

Does AppSync use defaults?

This comment has been minimized.

Copy link
@yyoshiki41

yyoshiki41 Mar 14, 2020

Author

@bboure

No, AppSync does not have default values.
error occurred when one of these fields is undefined.

CloudFormation Error

2020-03-14 19:00:27 UTC+0900
GraphQlDsDelta
UPDATE_FAILED
Property validation failure:
  [The property {/DynamoDBConfig/DeltaSyncConfig/BaseTableTTL} is required]

We need to set all of params baseTableTTL, deltaSyncTableName and deltaSyncTableTTL, if versioned is true.

This comment has been minimized.

Copy link
@bboure

bboure Mar 14, 2020

Collaborator

Would it make sense to add some defaults ourselves?
At least for TTL values? Not sure what would be a good value.

Additionally, we could throw an error when tableName or TTL values (if we don't use defaults) are undefined, before it gets to Cfn.

This comment has been minimized.

Copy link
@yyoshiki41

yyoshiki41 Mar 14, 2020

Author

OK, I'll prepare TTL default values.
(There is no value that can cover all cases, but I think it is not harmful.)

  • baseTableTTL: 0
    The amount of time (in minutes) items should be kept in the base table when deleted.
    Set to 0 to delete items in the base table immediately.
  • deltaSyncTableTTL: 60
    DeltaSync table will keep track of changes in 60 mins.
    NOTE: With a high velocity of data, it may make sense to keep this short. Alternatively, if your clients are offline for longer periods of time, it might be prudent to keep this longer.

Additionally, we could throw an error when tableName or TTL values (if we don't use defaults) are undefined, before it gets to Cfn.

I'll implements the validation for deltaSyncConfig params.

Thanks for your review!

This comment has been minimized.

Copy link
@yyoshiki41

yyoshiki41 Mar 14, 2020

Author

set default TTL values
243527d

@yyoshiki41

This comment has been minimized.

Copy link
Author

yyoshiki41 commented Mar 20, 2020

@bboure
sorry to bother you.
Can you review this pr when you have time to spare?
Thanks!

BaseTableTTL: !config.deltaSyncConfig.baseTableTTL ?
0 : config.deltaSyncConfig.baseTableTTL,
DeltaSyncTableName: config.deltaSyncConfig.deltaSyncTableName,
DeltaSyncTableTTL: !config.deltaSyncConfig.deltaSyncTableTTL ?

This comment has been minimized.

Copy link
@bboure

bboure Mar 20, 2020

Collaborator

@yyoshiki41 this looks wrong. If you set 0 for deltaSyncTableTTL you would get 60.
Check for undefined instead

This comment has been minimized.

Copy link
@yyoshiki41

yyoshiki41 Mar 20, 2020

Author

@bboure
thanks for your review

use typeof operator in the below commit.
2f404d6

README.md Outdated
# deltaSyncConfig:
# baseTableTTL: 0 # (defalut, not required) # The amount of time (in minutes) items should be kept in the base table when deleted. Set to 0 to delete items in the base table immediately
# deltaSyncTableName: { Ref: MyTableDelta } # required # The Delta Sync table name
# deltaSyncTableTTL: 60 # (defalut, not required) # The amount of time (in minutes) the delta sync table will keep track of changes

This comment has been minimized.

Copy link
@bboure

bboure Mar 20, 2020

Collaborator

@yyoshiki41 where do the default come from ?
what are the min and max values ?
please fix typo too :) defalut = default

This comment has been minimized.

Copy link
@yyoshiki41

yyoshiki41 Mar 20, 2020

Author

@bboure

TTL limitations

Max limitation are not defined.
Min limitations are below.
0 ≦ baseTableTTL
60 ≦ deltaSyncTableTTL

where do the default come from ?

I searched aws docs but default values is not mentioned in official.
So I adopted min ttl values as default.


fixed typo 🙇
6e4c551

@bboure
bboure approved these changes Mar 20, 2020
Copy link
Collaborator

bboure left a comment

@yyoshiki41 LGTM, I would appreciate to see a unit test thought.
Could you add it?
Then it's good to merge!

@yyoshiki41

This comment has been minimized.

Copy link
Author

yyoshiki41 commented Mar 20, 2020

@bboure

I'm not an expert on javascript and not familiar with jest tests.
Sorry that I couldn't help you..

@bboure

This comment has been minimized.

Copy link
Collaborator

bboure commented Mar 25, 2020

Closed in favour of #315

@bboure bboure closed this Mar 25, 2020
@bboure bboure mentioned this pull request Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.