Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

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

Merged
gregfurman merged 6 commits intomasterfrom
fix/esm/sqs-payload
Nov 29, 2024
Merged

[ESM] Fix polling of SQS queue when batch size exceeds 10#11945
gregfurman merged 6 commits intomasterfrom
fix/esm/sqs-payload

Conversation

@gregfurman
Copy link
Copy Markdown
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
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

@gregfurman gregfurman Nov 29, 2024

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
Copy Markdown
Contributor Author

@gregfurman gregfurman Nov 29, 2024

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
Copy Markdown
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
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
Copy Markdown
Member

@joe4dev joe4dev Nov 29, 2024

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
Copy Markdown
Member

@joe4dev joe4dev Nov 29, 2024

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
Copy Markdown
Member

@joe4dev joe4dev Nov 29, 2024

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
@gregfurman gregfurman deleted the fix/esm/sqs-payload branch November 29, 2024 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

2 more reviewers

@joe4dev joe4dev joe4dev approved these changes

@dfangl dfangl dfangl approved these changes

Reviewers whose approvals may not affect merge requirements

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