added stream support #5

Love how simple the new input is. Just a boolean. changing this input would also update the table, not only create. Is that correct? I think so from the code, but wanted to double check.

Just wanna make sure the table state is idempotent. Another edge case is, if the user changed the stream config using the console, after deployment, whatever in the yaml file should still be applied.

utils.js Show resolved Hide resolved

@dodgeblaster


 added stream turnerary in updateTable

acf543d

@danielcondemarin

Copy link

danielcondemarin commented Sep 26, 2019 •

edited

Loading

@dodgeblaster @eahefnawy Can I chip in and ask for making StreamViewType: 'NEW_AND_OLD_IMAGES' the default? This covers all bases and is also a requirement for the a global dynamo component I'm working on 🙏

eahefnawy

eahefnawy requested changes

Oct 29, 2019

View reviewed changes

Copy link

Member

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dodgeblaster ... looks good. Could you just update the README with this new input?

@tobilg

Copy link

tobilg commented Dec 2, 2019

Any chance that there's an ETA for this?

@dodgeblaster

Copy link

Contributor Author

dodgeblaster commented Dec 2, 2019

Any chance that there's an ETA for this?
I'll take a look at this tonight

dodgeblaster added 5 commits

December 2, 2019 19:22

@dodgeblaster


 commit-before-merge

aa4bf7f

@dodgeblaster


 added streamViewTypes as an input

2c2e417

@dodgeblaster


 cleaned up stream code, added example to README

46a675e

@dodgeblaster


 updated test yml file

68a7b58

@dodgeblaster


 added serverless.js and utils to commit

7ac573a

dodgeblaster

dodgeblaster commented

Dec 6, 2019

View reviewed changes

utils.js

Comment on lines +3 to +24

const validate = {

streamViewType: (inputs) => {

if (!inputs.streamViewType) {

return

}

const validStreamTypes = ['NEW_IMAGE', 'OLD_IMAGE', 'NEW_AND_OLD_IMAGES', 'KEYS_ONLY']

if (!validStreamTypes.includes(inputs.streamViewType)) {

throw Error(`${inputs.streamViewType} is not a valid streamViewType.`)

}

streamViewTypeUpdate: (comp, previousTable, inputs) => {

if (!previousTable.streamArn || !inputs.streamViewType) {

return

}

if (comp.state.stream && inputs.stream && comp.state.streamViewType !== inputs.streamViewType) {

throw Error(`You cannot change the view type of an existing DynamoDB stream.`)

}

Copy link

Contributor Author

@dodgeblaster dodgeblaster Dec 6, 2019 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added validation around stream types for creating and updating tables.
streamViewType just confirms they are using a correct view type, and will throw an error if they are not

streamViewTypeUpdate is a bit more subtle. If you change the viewType on an table that already has a stream, it wont do anything. Its possible people might assume it all worked unless they see a message explaining nothing actually happened. So I made an error for it.

@eahefnawy is throwing errors the best way to give messages like this in the CLI?

dodgeblaster

dodgeblaster commented

Dec 6, 2019

View reviewed changes

utils.js

Comment on lines +26 to +46

async function getStreamArn ({dynamodb, name}) {

const maxTries = 5

let tries = 0

const getStreamArn = async () => {

if (tries > maxTries) {

throw Error(`There was a problem getting the arn for your DynamoDB stream. Please try again.`)

}

const {streamArn } = await describeTable({ dynamodb, name})

if (!streamArn && tries <= maxTries) {

tries++

const sleep = ms => new Promise(r => setTimeout(r,ms))

await sleep(3000)

return await getStreamArn()

}

return streamArn

}

return await getStreamArn()

}

Copy link

Contributor Author

@dodgeblaster dodgeblaster Dec 6, 2019 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used in the update table function below.
I found that if I created a table with a stream, I would get returnValueFromCreateTable.TableDescription.LatestStreamArn successfully. But if I created a table with no stream, then set the stream to true in an update, the return value of the updateTable sdk call woud have returnValueFromUpdatTable.TableDescription.LatestStreamArn set to undefined (I am assuming that is because it takes a bit of time to register?). Running sls a second time will correctly give you the stream arn.

Anyways, that is a problem, because users will likely depend on getting a stream arn back from the outputs of this component. So the above function will try to get the stream arn, if it doesnt, it will wait 3 seconds and try again. It will do this 5 more times until it gets a stream arn. If it does not get it in 5 tries, it will throw an error.

The error does not mean the stream was not created, it just means we did not get the stream arn, which might be really important if the user is depending on the stream arn in other components.

dodgeblaster

dodgeblaster commented

Dec 6, 2019

View reviewed changes

utils.js

Comment on lines +106 to +110

...(disableStream && {

StreamSpecification: {

StreamEnabled: false

}

}),

Copy link

Contributor Author

@dodgeblaster dodgeblaster Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive added this disableStream flag here because the SDK will error if you try to disable a non existing stream. So this flag insures we only add this to the object if the inputs are disabling a stream that exists.

@dodgeblaster dodgeblaster requested a review from eahefnawy

December 11, 2019 05:17

@corentind59

Copy link

corentind59 commented Jan 28, 2021

Hello, will this PR be merged ? Or is it too far from current git history ? @dodgeblaster @eahefnawy ?

Labels

None yet

6 participants

@dodgeblaster @danielcondemarin @tobilg @corentind59 @ryanwalters @eahefnawy

Uh oh!

added stream support #5

Are you sure you want to change the base?

added stream support #5

Uh oh!

Conversation

@dodgeblaster dodgeblaster commented Aug 26, 2019

Uh oh!

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielcondemarin commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

Uh oh!

tobilg commented Dec 2, 2019

Uh oh!

dodgeblaster commented Dec 2, 2019

Uh oh!

@dodgeblaster dodgeblaster Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@dodgeblaster dodgeblaster Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@dodgeblaster dodgeblaster Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

corentind59 commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danielcondemarin commented Sep 26, 2019 •

edited

Loading

@dodgeblaster dodgeblaster Dec 6, 2019 •

edited

Loading

@dodgeblaster dodgeblaster Dec 6, 2019 •

edited

Loading