Add support for `maximumEventAge` and `maximumRetryAttempts` directly on function instead of destinations #7987

pgrzesik · 2020-07-25T11:47:15Z

Adds support for defining maximumEventAge and maximumRetryAttempts directly on function level and deprecates the configuration on destinations.

Closes: #7678

Open questions:

I wasn't sure about the approach to deprecation - I went with two separate deprecations for each parameter, but maybe it would make more sense to combine it into one instead? I would love to get your opinion on this one
I followed the previous approach where I didn't notice any validation of the provided input e.g. if maximumEventAge is between 60 seconds and 6 hours. Would it make sense to add such validation or it's unnecessary at this point?

Adds support for defining maximumEventAge and maximumRetryAttempts directly on function level Adds deprecation of setting the above parameters on destinations

codecov-commenter · 2020-07-25T11:49:19Z

Codecov Report

Merging #7987 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7987      +/-   ##
==========================================
- Coverage   88.13%   88.13%   -0.01%     
==========================================
  Files         242      244       +2     
  Lines        9146     9186      +40     
==========================================
+ Hits         8061     8096      +35     
- Misses       1085     1090       +5

Impacted Files	Coverage Δ
lib/plugins/aws/package/compile/functions/index.js	`97.21% <100.00%> (+0.14%)`	⬆️
lib/plugins/aws/provider/awsProvider.js	`92.74% <0.00%> (-0.09%)`	⬇️
lib/plugins/create/create.js	`90.69% <0.00%> (ø)`
lib/utils/awsSdkPatch.js	`50.00% <0.00%> (ø)`
lib/utils/getEnsureArtifact.js	`89.47% <0.00%> (ø)`
lib/plugins/aws/invokeLocal/index.js	`69.56% <0.00%> (+0.10%)`	⬆️
lib/plugins/aws/customResources/generateZip.js	`100.00% <0.00%> (+10.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6216f35...9cd2e10. Read the comment docs.

medikoo

@pgrzesik thank you, looks very good! I've proposed some suggestions (nothing major)

medikoo · 2020-07-27T07:33:39Z

docs/deprecations.md

+<a name="AWS_FUNCTION_DESTINATIONS_MAXIMUM_EVENT_AGE"><div>&nbsp;</div></a>
+
+## AWS Lambda Function Destinations `maximumEventAge`
+
+Please use `maximumEventAge` instead of `destinations.maximumEventAge`. Support for `destinations.maximumEventAge` will be removed with v2.0.0
+
+<a name="AWS_FUNCTION_DESTINATIONS_MAXIMUM_RETRY_ATTEMPTS"><div>&nbsp;</div></a>
+
+## AWS Lambda Function Destinations `maximumRetryAttempts`
+
+Please use `maximumRetryAttempts` instead of `destinations.maximumRetryAttempts`. Support for `destinations.maximumRetryAttempts` will be removed with v2.0.0
+


For simplicity let's use one deprecation, we can word it as:

maximumEventAge and maximumEventAge should be defined directly at function level. Support for those settings on destinations level, will be removed with v2.0.0

And code could probably be AWS_FUNCTION_DESTINATIONS_ASYNC_CONFIG

Great idea 💯

medikoo · 2020-07-27T07:36:59Z

docs/providers/aws/guide/functions.md

      onFailure: arn:aws:sns:us-east-1:xxxx:some-topic-name
 ```
+
+## Maximum Event Age and Maximum Retry Attempts for Asynchronous Invocations


Let's maybe group the async config sections as follows:

## Asynchronous invocation When intention is to invoke function asynchronously you may want to configure following additional settings ### Destinations [destination targets](https://docs.aws.amazon.com/lambda/latest/dg/invocation-async.html#invocation-async-destinations) can be the other... ... ### Maximum Event Age and Maximum Retry Attempts `maximumEventAge` accepts values between 60 ...

medikoo · 2020-07-27T07:37:24Z

lib/plugins/aws/package/compile/functions/index.js

+    const destinationConfig = {};
+
+    if (destinations) {
+      if (destinations.maximumRetryAttempts !== undefined) {


Let's write it as: if (destinations.maximumRetryAttempts != null) (naturally we should discard both null and undefined

medikoo · 2020-07-27T07:38:10Z

lib/plugins/aws/package/compile/functions/index.js

+        maximumRetryAttemptsOnDestinations = destinations.maximumRetryAttempts;
+      }
+
+      if (destinations.maximumEventAge !== undefined) {


Here as all supported values are not falsy, check can be even simpler if (destinations.maximumEventAge)

medikoo · 2020-07-27T07:42:43Z

lib/plugins/aws/package/compile/functions/index.js

+    const maximumEventAge = maximumEventAgeOnFunction || maximumEventAgeOnDestinations;
+    // For maximumRetryAttempts we cannot rely on "||" as the value can be Falsy, 0
+    const maximumRetryAttempts =
+      maximumRetryAttemptsOnFunction === undefined


Let's use == null comparision

medikoo · 2020-07-27T07:44:07Z

lib/plugins/aws/package/compile/functions/index.js

+        MaximumEventAgeInSeconds: maximumEventAge,
+        MaximumRetryAttempts: maximumRetryAttempts,


We should set those properties only if provided.

While above is harmless for undefined, if someone put null, it'll break CF deployment, while in such case we should ensure that no such property is set

pgrzesik · 2020-07-29T07:53:54Z

@medikoo Thanks a lot for valuable and comprehensive code review with great tips - I pushed suggested changes - if there's anything else that you feel could be improved, I'm all ears (or eyes in this case) 💯

medikoo

@pgrzesik thank you, looks great! I've proposed just few minor improvements to documentation, and we should be good to go

medikoo · 2020-07-29T07:56:28Z

docs/deprecations.md

+
+## AWS Lambda Function Destinations `maximumEventAge` & `maximumRetryAttempts`
+
+`maximumEventAge` and `maximumRetryAttempts` should be defined directly at function level. Support for those settings on destinations level, will be removed with v2.0.0


Let's put destinations in backticks

medikoo · 2020-07-29T07:57:42Z

docs/providers/aws/guide/functions.md

+
+### Maximum Event Age and Maximum Retry Attempts for Asynchronous Invocations
+
+When intention is to invoke function asynchronously you may want to configure `maximumRetryAttempts` and/or `maximumEventAge` for it.


Sentence When intention is.. is duplicated among sections, therfore maybe organize this as follows:

## Asynchronous invocation When intention is to invoke function asynchronously you may want to configure following additional settings ### Destinations [destination targets](https://docs.aws.amazon.com/lambda/latest/dg/invocation-async.html#invocation-async-destinations) can be the other... ... ### Maximum Event Age and Maximum Retry Attempts `maximumEventAge` accepts values between 60 ...

pgrzesik · 2020-07-29T08:17:21Z

Thanks @medikoo for such a quick re-review 🙇 Sorry for not catching these the first time around, hopefully everything should be good to go now 🤞

medikoo

Thank you @pgrzesik !

pgrzesik added 3 commits July 25, 2020 13:17

feat(AWS asynchronous invocations): support config on function level

e730767

Adds support for defining maximumEventAge and maximumRetryAttempts directly on function level Adds deprecation of setting the above parameters on destinations

docs: document destination params deprecations

e61be1e

docs: document support for asynchronous invocation parameters

fce805f

medikoo suggested changes Jul 27, 2020

View reviewed changes

medikoo added cat/lambda enhancement labels Jul 27, 2020

medikoo assigned pgrzesik Jul 27, 2020

pgrzesik added 3 commits July 28, 2020 21:50

Update deprecations docs

ec501a9

Update guide docs

86d4b43

feat(AWS asynchronous invocations): set properties only if defined

a2cbe5f

medikoo suggested changes Jul 29, 2020

View reviewed changes

docs(AWS asynchronous invocations): documentation cleanup/adjustments

9cd2e10

medikoo approved these changes Jul 29, 2020

View reviewed changes

medikoo merged commit 8573ec1 into serverless:master Jul 29, 2020

This was referenced Aug 18, 2020

AWS Now Supports Max Retry Attempts #7012

Closed

Remove support for maximumEventAge and maximumRetryAttempts at functions[].destinations #8119

Closed

lmmx mentioned this pull request Apr 18, 2022

Lambda client async ("Event") invocation does not obey configured retries boto/boto3#3225

Closed

		MaximumEventAgeInSeconds: maximumEventAge,
		MaximumRetryAttempts: maximumRetryAttempts,


		## AWS Lambda Function Destinations `maximumEventAge` & `maximumRetryAttempts`

		`maximumEventAge` and `maximumRetryAttempts` should be defined directly at function level. Support for those settings on destinations level, will be removed with v2.0.0


		### Maximum Event Age and Maximum Retry Attempts for Asynchronous Invocations

		When intention is to invoke function asynchronously you may want to configure `maximumRetryAttempts` and/or `maximumEventAge` for it.

Add support for maximumEventAge and maximumRetryAttempts directly on function instead of destinations #7987

Add support for maximumEventAge and maximumRetryAttempts directly on function instead of destinations #7987

Uh oh!

Conversation

pgrzesik commented Jul 25, 2020

Uh oh!

codecov-commenter commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgrzesik commented Jul 29, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

medikoo Jul 29, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

pgrzesik commented Jul 29, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add support for `maximumEventAge` and `maximumRetryAttempts` directly on function instead of destinations #7987

Add support for `maximumEventAge` and `maximumRetryAttempts` directly on function instead of destinations #7987

codecov-commenter commented Jul 25, 2020 •

edited

Loading

medikoo Jul 29, 2020 •

edited

Loading