-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[ESM] Fix polling of SQS queue when batch size exceeds 10 #11945
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
Conversation
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/sqs_poller.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/sqs_poller.py
Outdated
Show resolved
Hide resolved
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 28m 22s ⏱️ - 21m 49s Results for commit d4314f9. ± Comparison against base commit a47e701. This pull request removes 946 and adds 5 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/sqs_poller.py
Outdated
Show resolved
Hide resolved
6b7cd9b
to
10645b6
Compare
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/sqs_poller.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/sqs_poller.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/sqs_poller.py
Outdated
Show resolved
Hide resolved
ee20489
to
7490ef5
Compare
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.
LGTM, thanks for providing a minimal fix and postponing the complete fix until after the next patch release!
TEST_LAMBDA_EVENT_SOURCE_MAPPING_SEND_MESSAGE = os.path.join( | ||
THIS_FOLDER, "functions/lambda_event_source_mapping_send_message.py" | ||
) | ||
TEST_LAMBDA_SQS_QUEUE_MIRROR = os.path.join(THIS_FOLDER, "functions/lambda_sqs_queue_mirror.py") |
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.
What file is this queue mirror one?
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.
Oh damn. That was a test file I created to mirror an SQS queue from 1 to another using ESM. Turned out to be overkill. Will remove this reference
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.
Removed
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.
What a small validation can all entail ... :)
Thanks for pushing testing coverage into more behavioral complex parity lands 🚀
@@ -1042,6 +1043,182 @@ def test_sqs_event_source_mapping( | |||
rs = aws_client.sqs.receive_message(QueueUrl=queue_url_1) | |||
assert rs.get("Messages", []) == [] | |||
|
|||
@pytest.mark.parametrize("batch_size", [15, 100, 1_000, 10_000]) |
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.
nice coverage 💯
they took ~45s on my M1, but could be slower after we implement the window mechanism, hence it's good to adjust the window for LocalStack here 💡
BatchSize=20, | ||
ScalingConfig={ | ||
"MaximumConcurrency": 2 | ||
}, # Prevent more than 2 concurrent SQS workers from being spun up at a time |
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.
nit: these refer to Lambda pollers I guess (also something we don't support in LS yet ;)
|
||
batches = [] | ||
|
||
def get_msg_from_q(): |
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.
idea for future: It feels we re-implement this pattern all over the place in different ways and should consider using an appropriate helper/fixture 🙈
Motivation
This PR fixes CRUD issue where an SQS ESM's batch size exceeding 10 elements raises an exception on polling.
Changes
BatchSize
of greater than10
elements.Testing