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.

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

fix(stepfunctions-tasks): confusion between multiple ways to run a Lambda #6796

Merged
merged 13 commits into from
Mar 27, 2020
Merged

fix(stepfunctions-tasks): confusion between multiple ways to run a Lambda #6796

merged 13 commits into from
Mar 27, 2020

Conversation

shivlaks
Copy link
Contributor

@shivlaks shivlaks commented Mar 18, 2020 •
edited
Loading

Commit Message

fix(stepfunctions-tasks): confusion between multiple ways to run a Lambda

The InvokeFunction Step Functions task is being marked as deprecated. It represents the legacy way to represent Lambda functions in Step Functions

The RunLambda task represents the recommended way to invoke Lambdas in Step Functions.
see: https://docs.aws.amazon.com/step-functions/latest/dg/connect-lambda.html

Examples in the README have been updated to use RunLambdaTask

Closes #4801

Commit End

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

@shivlaks
fix(stepfunctions): there are two ways to invoke a Lambda function in…
b8181d1 
… stepfunctions tasks

BREAKING CHANGE: `invokeFunction` is no longer available step functions task. use `runLambdaTask` instead as it provides the same functionality
@shivlaks shivlaks requested review from rix0rrr and nija-at March 18, 2020 16:38
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 18, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: b8181d1
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8d542ee
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ac681b2
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 9c2aa39
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@nija-at nija-at assigned shivlaks and unassigned nija-at Mar 19, 2020
nija-at
nija-at reviewed Mar 19, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Breaking change statement - Use upper camel case for InvokeFunction and RunLambdaTask

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 19, 2020

I think I also generally disagree with removing the class. There will be a lot of people we will be breaking, which we need to think of. Let's @deprecate, and remove from the docs and examples instead?

@nija-at
Copy link
Contributor

nija-at commented Mar 19, 2020

I think I also generally disagree with removing the class. There will be a lot of people we will be breaking, which we need to think of. Let's @deprecate, and remove from the docs and examples instead?

After a chat with @rix0rrr and some further though, he has convinced me that this is probably a more customer friendly way to changing this.
However, we should make it clear that no new features or improvements should be expected on this.
We should also traverse all of our documentation and examples code (including aws-cdk-examples) to find and replace InvokeFunction with RunLambda.

@jogold
Copy link
Contributor

jogold commented Mar 19, 2020 •
edited
Loading

Is the handling of Lambda service exceptions the same when using the states arn vs the full function arn? Are Lambda errors reported in both cases? The documentation is really not clear about this...

@shivlaks
Copy link
Contributor Author

Is the handling of Lambda service exceptions the same when using the states arn vs the full function arn? Are Lambda errors reported in both cases? The documentation is really not clear about this...

good question, I'll follow up on these today

@shivlaks
Copy link
Contributor Author

I think I also generally disagree with removing the class. There will be a lot of people we will be breaking, which we need to think of. Let's @deprecate, and remove from the docs and examples instead?

After a chat with @rix0rrr and some further though, he has convinced me that this is probably a more customer friendly way to changing this.
However, we should make it clear that no new features or improvements should be expected on this.
We should also traverse all of our documentation and examples code (including aws-cdk-examples) to find and replace InvokeFunction with RunLambda.

sure, this is where I started out with this change before our decision to remove it. I also think it's more friendly to deprecate it and encourage the preferred flow to allow users the option of using it.

I like the suggestion to add some wording around future development and maintenance. Will make these changes.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: c91dddd
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0b0c766
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 3aab4d9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@shivlaks shivlaks changed the title fix(stepfunctions): there are two ways to invoke a Lambda function in stepfunctions tasks fix(stepfunctions-tasks): confusion between multiple ways to run a Lambda Mar 25, 2020
@shivlaks
Copy link
Contributor Author

Is the handling of Lambda service exceptions the same when using the states arn vs the full function arn? Are Lambda errors reported in both cases? The documentation is really not clear about this...

good question, investigating!

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 3a1c29b
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@shivlaks
Copy link
Contributor Author

Is the handling of Lambda service exceptions the same when using the states arn vs the full function arn? Are Lambda errors reported in both cases? The documentation is really not clear about this...

good question, investigating!

@jogold

Looked into this a bit further

  • Lambda service exceptions are handled the same whether we define them through either mechanism
  • Lambda Errors are reported in both cases

I also reached out to a member of the Step Functions team who verified that they should be the same

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 17400f5
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@jogold
Copy link
Contributor

jogold commented Mar 27, 2020

Is the handling of Lambda service exceptions the same when using the states arn vs the full function arn? Are Lambda errors reported in both cases? The documentation is really not clear about this...

good question, investigating!

@jogold

Looked into this a bit further

  • Lambda service exceptions are handled the same whether we define them through either mechanism
  • Lambda Errors are reported in both cases

I also reached out to a member of the Step Functions team who verified that they should be the same

Great! Thank you.

nija-at
nija-at approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

LGTM.

It would be useful to customers if we can configure the retry policy and they will get this for free (with options to adjust).

This, however, is quite hard to implement in our current implementation. One more case to do this - #6715. If we changed the pattern to what is described here, retry policy for each type of task can be configured within it.

@shivlaks
Copy link
Contributor Author

LGTM.

It would be useful to customers if we can configure the retry policy and they will get this for free (with options to adjust).

This, however, is quite hard to implement in our current implementation. One more case to do this - #6715. If we changed the pattern to what is described here, retry policy for each type of task can be configured within it.

good point, I'll take a look into this next

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: e4f6bf7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@shivlaks shivlaks merged commit 7485448 into master Mar 27, 2020
@shivlaks shivlaks deleted the shivlaks/stepfunctions-invoke-function branch March 27, 2020 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr left review comments

@nija-at nija-at nija-at approved these changes

Assignees

@shivlaks shivlaks

Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws-stepfunctions-tasks: Confusion between RunLambdaTask and InvokeFunction
5 participants
@shivlaks @aws-cdk-automation @rix0rrr @nija-at @jogold