-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
aws_lambda_event_source_mapping: maximum_batching_window_in_seconds #10051
aws_lambda_event_source_mapping: maximum_batching_window_in_seconds #10051
Conversation
Minor update, appears AWS updated the API to only accept SQS event source type mapping that have no maximum batching window set. Working with AWS on an open ticket if this is expected as modifying the request to include or exclude the property seems unnecessary. for those interested:
|
@tiny-dancer Are you sure? I too encountered this error however it was simply the case that I needed the latest aws-cli installed. This parameter was added very recently, so old versions don't recognise it. See the linked feature request for my example. |
@petewilcock good call out with CLI version, will double check. |
@petewilcock confirmed request successfully working on a kinesis event source mapping in same cli version. It appears closely aligned with the public announcement they added up front validation to their event source mapping API's, prior to the public announcement it was a little more liberal. $ aws lambda update-event-source-mapping --uuid xxxx --function-name xxx --enabled --batch-size 10 --maximum-batching-window-in-seconds 0
{
"UUID": "xxx",
"BatchSize": 100,
"MaximumBatchingWindowInSeconds": 299,
"EventSourceArn": "xxx",
"FunctionArn": "xxx",
"LastModified": 1569944880.0,
"LastProcessingResult": "PROBLEM: Function call failed",
"State": "Updating",
"StateTransitionReason": "User action"
} Also encountered this in the golang SDK via terraform, intercepted API Request: Request: {
"BatchSize": 10,
"Enabled": true,
"FunctionName": "arn:aws:lambda:us-east-1:xxx:function:xxx",
"MaximumBatchingWindowInSeconds": 0
} Response: {
"Type": "User",
"message": "Unsupported MaxiumumBatchingWindowInSecond parameter for given event source mapping type."
} The same terraform binary successfully updating a Kinesis event source mapping: |
Confirmed via AWS. |
Is there an ETA for getting this change reviewed and merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tiny-dancer 👋 Thanks so much for this. Overall its looking really good. Two feedback items and this should be good to go. Please reach out if you have any questions or do not have time to implement them. 👍
testAccCheckAwsLambdaEventSourceMappingExists(resourceName, &conf), | ||
resource.TestCheckResourceAttr(resourceName, "maximum_batching_window_in_seconds", strconv.Itoa(int(batchWindow))), | ||
), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if there was an additional test step to verify updating maximum_batching_window_in_seconds
👍
Co-Authored-By: Brian Flad <bflad417@gmail.com>
// AWS API will fail if this parameter is set (even as default value) for sqs event source. Ideally this should be implemented in GO SDK or AWS API itself. | ||
eventSourceArn, err := arn.Parse(d.Get("event_source_arn").(string)) | ||
|
||
if err == nil && eventSourceArn.Service != "sqs" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bflad your proposed change swallows the err
here if it were to exist, was that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vs returning the err
or printing a warning of sorts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if this was confusing, I should have elaborated on swallowing the error -- if anything, potentially returning a warning log would feel appropriate here since the ARN should have gone through multiple forms of validation at this point (either from Terraform or the API itself). Returning an error could leave operators in a potentially unrecoverable state if for some reason the event source ARN was not an ARN and they made it this far. Hope this makes sense. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense - thanks for the explanation!
@bflad thanks for the review! addressed feedback and left the let me know if there's anything else! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks, @tiny-dancer 🚀
Output from acceptance testing:
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_withFunctionName (59.92s)
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_disappears (70.86s)
--- PASS: TestAccAWSLambdaEventSourceMapping_StartingPositionTimestamp (72.83s)
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_removeBatchSize (84.38s)
--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (84.60s)
--- PASS: TestAccAWSLambdaEventSourceMapping_BatchWindow (85.49s)
--- PASS: TestAccAWSLambdaEventSourceMapping_sqsDisappears (109.06s)
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (122.73s)
--- PASS: TestAccAWSLambdaEventSourceMapping_changesInEnabledAreDetected (132.31s)
This has been released in version 2.39.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Release note for CHANGELOG:
From AWS Docs:
By default, Lambda invokes your function as soon as records are available in the stream. If the batch it reads from the stream only has one record in it, Lambda only sends one record to the function. To avoid invoking the function with a small number of records, you can tell the event source to buffer records for up to 5 minutes by configuring a batch window. Before invoking the function, Lambda continues to read records from the stream until it has gathered a full batch, or until the batch window expires.
Output from acceptance testing: