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

(trigger): Allow trigger to work with Lambda functions with long timeouts #23058

Open
1 of 2 tasks
DerkSchooltink opened this issue Nov 23, 2022 · 1 comment
Open
1 of 2 tasks
Assignees
Labels
@aws-cdk/triggers Related to the triggers package effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@DerkSchooltink
Copy link
Contributor

DerkSchooltink commented Nov 23, 2022

Describe the feature

Considering the following resources:

const lambda = new GoFunction(this, 'LambdaWithLongTimeout', {
    entry: '../../test',
    timeout: Duration.minutes(15),
});

new Trigger(this, 'LambdaTrigger', {
    handler: lambda,
});

Considering the timeout of 15 minutes of the Lambda to trigger on build, I would expect the trigger to be able to deal with this. However, the SDK in the trigger Lambda times out after 2 minutes:

(LambdaTriggerLambdaWithLongTimeoutA4E48B8F) Received response status [FAILED] from custom resource. Message returned: TimeoutError: Connection timed out after 120000ms

I see two potential solutions:

  1. Add the option to specify an override for the Lambda SDK client config timeout in the trigger Lambda
  2. Add the option to specify an override for the invocation type (to Event) of the Lambda to be triggered

Or a combination of both as optional parameters! I can see a use-case for specifying invocation type Event for a fire-and-forget trigger, separate from increasing timeouts on the trigger Lambda.

Use Case

For Lambda functions that run longer than 2 minutes, I want to execute a trigger for them at build-time. There is no reason to assume all Lambda functions finish within this 2 minute window.

Proposed Solution

Considering the previous resources, I propose a solution that would allow the following:

const lambda = new GoFunction(this, 'LambdaWithLongTimeout', {
    entry: '../../test',
    timeout: Duration.minutes(15),
});

new Trigger(this, 'LambdaTrigger', {
    handler: lambda,
    timeout: Duration.minutes(15),
    invocationType: 'Event',
});

Where timeout and invocationType are both optional parameters in the properties of Trigger.

If timeout is not set, it will default to the default timeout of the AWS SDK configuration timeout (2 minutes).

If invocationType is not set, it will default RequestResponse, which is a default for creating Lambdas, and guarantees backwards compatibility with existing usages of Trigger.

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.51.1

Environment details (OS name and version, etc.)

MacOs Ventura 13.0.1

@DerkSchooltink DerkSchooltink added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Nov 23, 2022
@github-actions github-actions bot added the @aws-cdk/triggers Related to the triggers package label Nov 23, 2022
@pahud pahud added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Nov 23, 2022
@pahud
Copy link
Contributor

pahud commented Nov 23, 2022

Yeah I believe it should be possible to expose the timeout property to the surface in TriggerOptions or TriggerProps. Thanks for that and we'll be happy to review the PR.

@pahud pahud added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Nov 23, 2022
mergify bot pushed a commit that referenced this issue Dec 16, 2022
…timeouts (#23062)

Implements #23058
----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Jan 20, 2023
…timeouts (aws#23062)

Implements aws#23058
----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Feb 22, 2023
…timeouts (aws#23062)

Implements aws#23058
----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit that referenced this issue Apr 5, 2023
…timeouts (#24435)

Reopened PR from #23650 and #23788

And re-revert from #23062 ([v2.61.1](https://github.com/aws/aws-cdk/releases/tag/v2.61.1))

Implements #23058 and fixes #23407

Worked on comments from @TheRealAmazonKendra as well. Added an integration test scenario where a Lambda function will be triggered by a Trigger construct, send a message to an SQS queue, which will be consumed by the integration test assertion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/triggers Related to the triggers package effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

3 participants