Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 29, 2024
Merged

[ESM] Fix polling of SQS queue when batch size exceeds 10 #11945

merged 6 commits into from
Nov 29, 2024

Conversation

gregfurman
Copy link
Contributor

@gregfurman gregfurman commented Nov 27, 2024 •
edited
Loading

Motivation

This PR fixes CRUD issue where an SQS ESM's batch size exceeding 10 elements raises an exception on polling.

Changes

  • Fixes bug when creating an SQS mapping that prevented a BatchSize of greater than 10 elements.

Testing

  • Added snapshot tests for varying batch sizes.

@gregfurman gregfurman added type: bug Bug report semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases aws:lambda:event-source-mapping AWS Lambda Event Source Mapping (ESM) labels Nov 27, 2024
@gregfurman gregfurman self-assigned this Nov 27, 2024
@gregfurman gregfurman added semver: patch Non-breaking changes which can be included in patch releases and removed semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Nov 27, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 27, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±  0      2 suites  ±0   1h 28m 22s ⏱️ - 21m 49s
2 837 tests  - 941  2 588 ✅  - 844  249 💤  - 97  0 ❌ ±0 
2 839 runs   - 941  2 588 ✅  - 844  251 💤  - 97  0 ❌ ±0 

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. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_sqs.TestSQSEventSourceMapping ‑ test_sqs_event_source_mapping_batch_size[10000]
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_sqs.TestSQSEventSourceMapping ‑ test_sqs_event_source_mapping_batch_size[1000]
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_sqs.TestSQSEventSourceMapping ‑ test_sqs_event_source_mapping_batch_size[100]
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_sqs.TestSQSEventSourceMapping ‑ test_sqs_event_source_mapping_batch_size[15]
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_sqs.TestSQSEventSourceMapping ‑ test_sqs_event_source_mapping_batching_reserved_concurrency

♻️ This comment has been updated with latest results.

@gregfurman gregfurman marked this pull request as ready for review November 27, 2024 14:25
@joe4dev joe4dev mentioned this pull request Nov 28, 2024
Closed
1 task
@gregfurman gregfurman added this to the 4.0.3 milestone Nov 29, 2024
@gregfurman gregfurman changed the title [ESM] Create SQS message collector for batched processing [ESM] Fix polling of SQS queue when batch size exceeds 10 Nov 29, 2024
dfangl
dfangl approved these changes Nov 29, 2024
View reviewed changes
Copy link
Member

@dfangl dfangl left a 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!

tests/aws/services/lambda_/test_lambda.py Outdated
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")
Copy link
Member

@dfangl dfangl Nov 29, 2024 •
edited
Loading

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

joe4dev
joe4dev approved these changes Nov 29, 2024
View reviewed changes
Copy link
Member

@joe4dev joe4dev left a 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 🚀

tests/aws/services/lambda_/event_source_mapping/test_lambda_integration_sqs.py
@@ -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])
Copy link
Member

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 💡

tests/aws/services/lambda_/event_source_mapping/test_lambda_integration_sqs.py
BatchSize=20,
ScalingConfig={
"MaximumConcurrency": 2
}, # Prevent more than 2 concurrent SQS workers from being spun up at a time
Copy link
Member

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 ;)

tests/aws/services/lambda_/event_source_mapping/test_lambda_integration_sqs.py

batches = []

def get_msg_from_q():
Copy link
Member

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 🙈

@gregfurman gregfurman merged commit 55295bf into master Nov 29, 2024
32 checks passed
@gregfurman gregfurman deleted the fix/esm/sqs-payload branch November 29, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

@dfangl dfangl dfangl approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees

@gregfurman gregfurman

Labels
aws:lambda:event-source-mapping AWS Lambda Event Source Mapping (ESM) semver: patch Non-breaking changes which can be included in patch releases type: bug Bug report
Projects
None yet
Milestone
4.0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@gregfurman @joe4dev @dfangl