Skip to content

ESMv2: Fix UpdateEventSourceMapping request and batch size check for SQS #11637

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 5 commits into from
Oct 8, 2024
Merged

ESMv2: Fix UpdateEventSourceMapping request and batch size check for SQS #11637

merged 5 commits into from
Oct 8, 2024

Conversation

gregfurman
Copy link
Contributor

@gregfurman gregfurman commented Oct 4, 2024 •
edited by joe4dev
Loading

Motivation

This PR addresses some regressions in ESM v2 parity when compared with ESM v1.

Changes

  • UpdateEventSourceMapping requests now work -- where an update request will re-create a new ESM from scratch and overwrite the previous version with its new state.
  • validate_event_source_mapping has been extended to include UpdateEventSourceMapping requests

Testing

The following regressions have been remedied with the new changes:

  • test_event_source_mapping_default_batch_size
  • test_sqs_event_source_mapping_update

ext test runs:

@gregfurman gregfurman added aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases labels Oct 4, 2024
@gregfurman gregfurman added this to the 3.8 milestone Oct 4, 2024
@gregfurman gregfurman self-assigned this Oct 4, 2024
gregfurman
gregfurman commented Oct 4, 2024
View reviewed changes
localstack-core/localstack/services/lambda_/api_utils.py
@@ -637,7 +637,7 @@ def validate_and_set_batch_size(service: str, batch_size: Optional[int] = None)
BATCH_SIZE_RANGES = {
"kafka": (100, 10_000),
"kinesis": (100, 10_000),
"dynamodb": (100, 1_000),
"dynamodb": (100, 10_000),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.aws.amazon.com/lambda/latest/api/API_CreateEventSourceMapping.html#API_CreateEventSourceMapping_RequestBody

Amazon DynamoDB Streams – Default 100. Max 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.

@gregfurman Can we trigger the ext pipeline here to check whether it breaks pipes?

(if urgent, better compromise at Pipes in preview rather than ESM)

gregfurman
gregfurman commented Oct 4, 2024
View reviewed changes
localstack-core/localstack/services/lambda_/provider.py
elif service == "kafka":
if set(mapping["Topics"]).intersection(request["Topics"]):
# Check we are validating a CreateEventSourceMapping request
if is_create_esm_request:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only validate the below checks if the request is a CreateEventSourceMapping since this will check for duplicates etc.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 4, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±  0      2 suites  ±0   1h 20m 16s ⏱️ - 21m 19s
2 560 tests  - 927  2 244 ✅  - 828  316 💤  - 99  0 ❌ ±0 
2 562 runs   - 927  2 244 ✅  - 828  318 💤  - 99  0 ❌ ±0 

Results for commit 2f897d1. ± Comparison against base commit f6d2d07.

This pull request removes 927 tests. 
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]
…

♻️ This comment has been updated with latest results.

@alexrashed alexrashed modified the milestones: 3.8, 4.0 Oct 7, 2024
@gregfurman gregfurman force-pushed the fix/esm/regressions branch from 727e8a7 to 2f897d1 Compare October 7, 2024 15:08
@gregfurman gregfurman marked this pull request as ready for review October 7, 2024 15:09
dfangl
dfangl approved these changes Oct 8, 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 for the initial approach! There are definitely more improvements we need to make, like persisting the state of stream listeners, and more test for the various update states, but this should at least come close to the parity of the old implementation!

localstack-core/localstack/services/lambda_/provider.py

# We should stop() the worker since the delete() will remove the ESM from the state mapping.
esm_worker.stop()
# This will either create an EsmWorker in the CREATING state if enabled. Otherwise, the DISABLING state is set.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is a bit misleading, it will create an EsmWorker with the internal state of CREATING, but the state in the store depends on the actual action we are taking. In the followups, we should discuss the state management of ESM workers, I am not a fan at all with the current approach, but good job making the best out of it!

@dfangl dfangl merged commit 9a09737 into master Oct 8, 2024
35 checks passed
@dfangl dfangl deleted the fix/esm/regressions branch October 8, 2024 08:24
macnev2013 pushed a commit that referenced this pull request Oct 22, 2024
@gregfurman @macnev2013
ESMv2: Fix UpdateEventSourceMapping request and batch size check for …
2ab5135 
…SQS (#11637)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev left review comments

@dfangl dfangl dfangl approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

Assignees

@gregfurman gregfurman

Labels
aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
3.8
Development

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