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

Ensure the function in aws-node-express-dynamodb-api example to be idempotent #658

Closed
wants to merge 1,309 commits into from

Conversation

Nsupyq
Copy link
Contributor

@Nsupyq Nsupyq commented Nov 2, 2021

This PR tries to fix issue #657. It replaces the non-idempotent API dynamoDbClient.put with the idempotent API dynamoDbClient.transactWrite to make the function of writing username in index.js idempotent. Invoking dynamoDbClient.transactWrite needs a unique identifier called ClientRequestToken, which is used by DynamoDB to determine whether the incoming write operation is a retry. Thus this PR adds a function parameter called token to be ClientRequestToken.

eahefnawy and others added 30 commits March 23, 2020 15:11
…3-sigurl-obsolete-dependency

Fix aws python pynamodb s3 sigurl obsolete dependency (closes serverless#364)
…sqs-standard

New example of SQS with Typescript
Fix spelling of aws-node-signed-uploads
This commit fixes the title in the README.md file
Added region and profile support at `runAwsCommand`. User can specify `region` and/or `profile` in the provider section of serverless.yml.
Remove 'Serverless Q&a Example' from Community Examples
…yarn/aws-node-dynamic-image-resizer/acorn-5.7.4

Bump acorn from 5.7.3 to 5.7.4 in /aws-node-dynamic-image-resizer
Bumps [acorn](https://github.com/acornjs/acorn) from 6.1.1 to 6.4.1.
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@6.1.1...6.4.1)

Signed-off-by: dependabot[bot] <[email protected]>
…yarn/acorn-6.4.1

Bump acorn from 6.1.1 to 6.4.1
…yarn/aws-node-vue-nuxt-ssr/acorn-6.4.1

Bump acorn from 6.1.1 to 6.4.1 in /aws-node-vue-nuxt-ssr
…yarn/aws-node-dynamic-image-resizer/lodash-4.17.13

Bump lodash from 4.17.11 to 4.17.13 in /aws-node-dynamic-image-resizer
…yarn/aws-node-dynamic-image-resizer/mixin-deep-1.3.2

Bump mixin-deep from 1.3.1 to 1.3.2 in /aws-node-dynamic-image-resizer
…yarn/aws-node-dynamic-image-resizer/handlebars-4.7.6

Bump handlebars from 4.1.0 to 4.7.6 in /aws-node-dynamic-image-resizer
…yarn/aws-node-dynamic-image-resizer/js-yaml-3.13.1

Bump js-yaml from 3.12.2 to 3.13.1 in /aws-node-dynamic-image-resizer
…yarn/aws-node-auth0-custom-authorizers-api/lodash-4.17.15

Bump lodash from 4.17.5 to 4.17.15 in /aws-node-auth0-custom-authorizers-api
@mnapoli
Copy link
Contributor

mnapoli commented Nov 2, 2021

Hi! Looking at #657 and the PR, I wonder whether this is worth fixing? The example is really simple and small (as it's supposed to be). By adding in transactions, it makes it much more complex (e.g. now to play the example we need to send a token, which is not really clear how to get that token).

Is the example really supposed to be perfect at a high scale?

@pgrzesik
Copy link
Contributor

pgrzesik commented Nov 2, 2021

I share the same sentiment as @mnapoli here - looking at the change, it now seems more cryptic to users that are just starting out and that's the goal of these type of templates. Do you strongly feel that making the example more complex is worth it here?

Now the client does not need to provide a token. The requestId provided by AWS can be used as the ClientRequestToken.
@Nsupyq
Copy link
Contributor Author

Nsupyq commented Nov 3, 2021

Hi! I have simplified the idempotence check mechanism. Now we do not need to send a token. The requestId provided by AWS Lambda can be used as the ClientRequestToken.

@mnapoli
Copy link
Contributor

mnapoli commented Nov 3, 2021

Right, thank you.

To be honest, I have reservations. First, the code change that you introduced uses the AWS request ID as a token: are we 100% sure that request ID is the same in retries? (I would doubt that)

Second, now that we have the diff, and after re-reading #657, it seems to me this change isn't needed at all:

Assuming two concurrent functions want to change the user name with the same userId. When one of them fails after executing dynamodb_client.put, AWS Lambda will retry it

The example is an express app: this is an HTTP (API Gateway) event, which is synchronous. Lambda will not retry the invocation.

So I think there is no issue to fix here?

@Nsupyq
Copy link
Contributor Author

Nsupyq commented Nov 3, 2021

For your first question, AWS Lambda will use the same parameters to retry a function. The awsRequestId is part of the functional parameters and it is the same in retries.

For the second question, although this is an HTTP (API Gateway) event, the client can still invoke the function asynchronously via AWS CLI or some other ways. Then the function will still be retried.

Finally, idempotence is a new requirement for developers of serverless applications and is important for fault tolerance. I think it would be better to teach people about this thing at the beginning.

@mnapoli
Copy link
Contributor

mnapoli commented Nov 3, 2021

For the second question, although this is an HTTP (API Gateway) event, the client can still invoke the function asynchronously via AWS CLI or some other ways.

Yes, but that's not what the example is about. This is a starter example, it doesn't have to account for the example to be misused in scenarios with high load.

@mattwelke
Copy link
Contributor

mattwelke commented Nov 3, 2021

@Nsupyq raised an issue in my examples repository (https://github.com/mattwelke/serverless-managed-dbs-examples) too related to this. I agree, these examples are meant to be starters only. The idea is that beginners get up and running as quick as possible with their functions. And then they can explore more complex examples over time when they need complex things.

But I also wanted to say, I love that you're out there contributing to repos @Nsupyq. Have you considered creating your own third-party example, like my repo, showing more advanced concepts like idempotent functions?

@Nsupyq
Copy link
Contributor Author

Nsupyq commented Nov 4, 2021

Thank you @mattwelke @mnapoli :)
I will create my own examples to introduce what is idempotency and how to guarantee that.
I will propose new PRs when I finish them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.