8000 [ESM] Add backoff between Stream Poller retries by gregfurman · Pull Request #12281 · localstack/localstack · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[ESM] Add backoff between Stream Poller retries #12281

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 3 commits into from
Feb 19, 2025
Merged

[ESM] Add backoff between Stream Poller retries #12281

merged 3 commits into from
Feb 19, 2025

Conversation

gregfurman
Copy link
Contributor

Motivation

This PR leverages the backoff util to ensure, when retrying, we wait between retries so as to not spam the processing loop.

Companion PR to #12264

Changes

  • Checks has_record_expired every iteration to allow for an expired record to break processing.
  • Use backoffutility, in conjunction with a threading.Event, to exponentially back-off between processing retries.

@gregfurman gregfurman added semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases aws:pipes Amazon EventBridge Pipes aws:lambda:event-source-mapping AWS Lambda Event Source Mapping (ESM) labels Feb 18, 2025
@gregfurman gregfurman self-assigned this Feb 18, 2025
@gregfurman gregfurman requested a review from tiurin February 18, 2025 09:30
@gregfurman gregfurman marked this pull request as ready for review February 18, 2025 12:40
@gregfurman gregfurman marked this pull request as draft February 18, 2025 14:14
Base automatically changed from add/util/retry-backoff to master February 18, 2025 16:38
@gregfurman gregfurman force-pushed the add/esm/retry-backoff branch from 05becff to 1476ae8 Compare February 18, 2025 16:41
@gregfurman gregfurman marked this pull request as ready for review February 18, 2025 16:41
@github-actions GitHub Actions
Copy link
github-actions bot commented Feb 18, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±  0      2 suites  ±0   1h 29m 18s ⏱️ - 21m 56s
3 106 tests  - 993  2 886 ✅  - 881  220 💤  - 112  0 ❌ ±0 
3 108 runs   - 993  2 886 ✅  - 881  222 💤  - 112  0 ❌ ±0 

Results for commit dbcdde6. ± Comparison against base commit 9bc5bc9.

This pull request removes 993 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.TestLo
8000
anBrokerScenario ‑ 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.

joe4dev
joe4dev approved these changes Feb 19, 2025
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.

Nice application of the backoff utility 👍

It would be great to highlight the motivation on why we are doing this from a customer perspective (as discussed and raised by @dfangl ) in future PR descriptions.

localstack-core/localstack/services/lambda_/event_source_mapping/pollers/stream_poller.py Outdated
if not arrival_timestamp_of_last_event:
return False

now = get_current_time().timestamp()
Copy link
Member

Choose a reason for hiding this comment

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

Are we handling timezones correctly here?

localstack-core/localstack/services/lambda_/event_source_mapping/pollers/stream_poller.py Outdated
def pre_filter(self, events: list[dict]) -> list[dict]:
return events

def post_filter(self, events: list[dict]) -> list[dict]:
return events

def has_record_expired(self, event: dict):
Copy link
Member

Choose a reason for hiding this comment

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

That's an untested feature for which we should ideally add a test (which is a bit annoying because of the 60s minimum https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-eventsourcemapping.html#cfn-lambda-eventsourcemapping-maximumrecordageinseconds). Can we at least add a backlog item for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to revert these changes and make a follow-up PR with a test instead?

Copy link
Member

Choose a reason for hiding this comment

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

This is blocking for one of my PRs (as the logs are so massive they cannot reasonably be printed due to the retries), would be nice to get this in a separate PR, as there seem to be some confusion still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

dfangl
dfangl approved these changes Feb 19, 2025
View reviewed changes
Copy link
Member
@dfangl dfangl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, thanks for the fix! No more 500k log lines after the function was deleted :)

@gregfurman gregfurman merged commit 3b6bee2 into master Feb 19, 2025
31 checks passed
@gregfurman gregfurman deleted the add/esm/retry-backoff branch February 19, 2025 14:48
@gregfurman gregfurman mentioned this pull request Feb 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiurin tiurin tiurin approved these changes

@joe4dev joe4dev joe4dev approved these changes

@dfangl dfangl dfangl approved these changes

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

Assignees

@gregfurman gregfurman

Labels
aws:lambda:event-source-mapping AWS Lambda Event Source Mapping (ESM) aws:pipes Amazon EventBridge Pipes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
No milestone
Development

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