fix message retry in lambda SQS event source mapping #6603
Conversation
2f4092e to
4d2095c
Compare
There was a problem hiding this comment.
Some minor nits and one remark that might at least for localstack scope not actually be an issue (long vs. short polling).
It's a pity we can't use destinations for every lambda integration to keep track of successful and failed invocations 😕
Still I think the tests seem fairly robust (though I'm not sure if ApproximateReceiveCount might be flaky against AWS?) and the pattern that the lambda itself calls the queue and sends results this way, doesn't seem too weird to me. But maybe that's also just because I was working with this in the past where that was basically the only option 😉
In the future I think it would be interesting to see if we can instrument the test lambdas via the Extensions API and setup some sort of reusable reporting architecture that works on both AWS and localstack to keep track of successful and failed invocations.
💯 for removing legacy code from the event source listeners 😁
🚀
| result = sqs_client.receive_message( | ||
| QueueUrl=queue_url, | ||
| AttributeNames=["All"], | ||
| MessageAttributeNames=["All"], | ||
| MaxNumberOfMessages=batch_size, | ||
| ) |
There was a problem hiding this comment.
Not 100% sure but I think this needs to have a value set for WaitTimeSeconds since per default the SQS poller uses long polling (for non-FIFO queues at least) according to https://docs.aws.amazon.com/lambda/latest/dg/with-sqs.html#events-sqs-scaling
There was a problem hiding this comment.
In localstack that might not be particularly relevant though because AFAIK we don't differentiate between short and long polling anyway.
There was a problem hiding this comment.
totally agree. unfortunately we can't block here because of the way the loop is structured. see comment here: #5042 (comment)
that would require a thread per event-source-mapping, which again requires a different interface for the event source listener (adding/removing mappings). i'd love to do that as part of the lambda rework.
tests/integration/awslambda/functions/lambda_sqs_integration.py
Outdated
Show resolved
Hide resolved
tests/integration/awslambda/functions/lambda_sqs_integration.py
Outdated
Show resolved
Hide resolved
tests/integration/awslambda/functions/lambda_sqs_integration.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>
This PR fixes how the SQS event source listener deals with DLQs and retry intervals.
There was some very old code left from #1973 that implemented logic that would, for SQS triggered lambdas, put messages into a DLQ manually. This logic is not needed since SQS puts messages into a DLQ automatically when it's being polled (which the event source listener does). I removed all of that code, since it is all handled implicitly in our SQS provider.
For testing I created two new tests that test the basic behavior (which replace the defunct test i removed), and retry with a DLQ (which was the original goal for #5283). The SQS event source mapping does not support
DestinationConfig, meaning you can't make async lambdas triggered by SQS messages also send lambda results to SQS queues 🤷. But I needed that to write proper tests, so I wrote a simple new lambda for testing that takes as input a destination queue URL to which the lambda will echo the incoming event.The
sqs_api()transformer was causing issue because sometimes theSenderIdis the same as the account ID, but not always, so I exposed_resource_name_transformerthrough the transformer util so i can build some custom transformations. happy to revise that.I'll raise a follow-up PR to move the other lambda/sqs integration tests scattered throughout different tests into the new
test_lambda_sqs_integration.py.