Skip to content

[ESM] Handle DynamoDB-local Invalid ShardID exception #12036

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
Dec 13, 2024
Merged

[ESM] Handle DynamoDB-local Invalid ShardID exception #12036

merged 6 commits into from
Dec 13, 2024

Conversation

gregfurman
Copy link
Contributor

Motivation

Doing a GetRecords operation against DynamoDB-local can raise an (undocumented) Invalid ShardId in ShardIterator exception. Since we poll a stream in ESM, encountering this sharding issue should necessitate a re-initialise of the shards we are polling.

Changes

  • Check whether ResourceNotFound exception is of type Invalid ShardId in ShardIterator, and trigger a re-sharding if encountered.

@gregfurman gregfurman added type: bug Bug report semver: patch Non-breaking changes which can be included in patch releases labels Dec 13, 2024
@gregfurman gregfurman added this to the 4.1 milestone Dec 13, 2024
@gregfurman gregfurman requested a review from bentsku December 13, 2024 17:38
@gregfurman gregfurman self-assigned this Dec 13, 2024
@bentsku bentsku added the aws:dynamodbstreams AWS DynamoDB Streams label Dec 13, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 13, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±  0      2 suites  ±0   1h 30m 23s ⏱️ - 20m 10s
2 944 tests  - 950  2 731 ✅  - 853  213 💤  - 97  0 ❌ ±0 
2 946 runs   - 950  2 731 ✅  - 853  215 💤  - 97  0 ❌ ±0 

Results for commit e02a1af. ± Comparison against base commit 235a06a.

This pull request removes 950 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.

@gregfurman gregfurman marked this pull request as ready for review December 13, 2024 18:32
bentsku
bentsku approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku 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 a lot for tackling this! 🙏

I'll probably open a follow up soon with the TrimmedDataAccessException exception sometimes raised by DynamoDB-local and also considered to be a bug.

I think this is a safe fix, and if anything goes wrong with Dynamodb-local, it will also flood the logs, so we should be good to go. Hopefully in the future we can rework the sharding behavior!

bentsku
bentsku reviewed Dec 13, 2024
View reviewed changes
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/stream_poller.py Outdated
except self.source_client.exceptions.ExpiredIteratorException as e:
except (
self.source_client.exceptions.ExpiredIteratorException,
self.source_client.exceptions.TrimmedDataAccessException,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's nice, I thought we'd need to do string matching again, but both dynamodbstreams and the kinesis clients implements the TrimmedDataAccessException? That's practical 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kinesis doesn't raise this ://// will re-implement

bentsku
bentsku reviewed Dec 13, 2024
View reviewed changes
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/dynamodb_poller.py Outdated
def get_records(self, shard_iterator: str) -> dict:
try:
return super().get_records(shard_iterator)
except self.source_client.exceptions.TrimmedDataAccessException as e:
Copy link
Contributor

@bentsku bentsku Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is not going to be caught because self.source_client.exceptions.TrimmedDataAccessException is a subclass from ClientError, and the parent class get_records is already catching that, and if not a "recognized" exception, raised PipeInternalError from e. Not sure if we can catch that somehow? Ugh, sorry for this one... 😓

Copy link
Contributor

@bentsku bentsku Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess we could try to catch PipeInternalError, verify if it is raised from the exception we want (not even sure how to do that 👀 ) and if not, re-raise? Not great... thanks, DynamoDB-local 😅

Maybe it'd be easier to do a string matching in the parent get_records like Invalid ShardId in ShardIterator but matching TrimmedDataAccessException?

bentsku
bentsku reviewed Dec 13, 2024
View reviewed changes
localstack-core/localstack/services/lambda_/event_source_mapping/pollers/stream_poller.py Outdated
@@ -264,7 +264,7 @@ def get_records(self, shard_iterator: str) -> dict:
)
return get_records_response
# TODO: test iterator expired with conditional error scenario (requires failure destinations)
except self.source_client.exceptions.ExpiredIteratorException as e:
except self.source_client.exceptions.ExpiredIteratorExceptionas as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
except self.source_client.exceptions.ExpiredIteratorExceptionas as e:
except self.source_client.exceptions.ExpiredIteratorException as e:

@gregfurman gregfurman force-pushed the fix/esm/dynamodb-no-shard branch from 451256b to ef48cb4 Compare December 13, 2024 19:19
bentsku
bentsku approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku 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 a lot really for adding the TrimmedDataAccessException too 🙇

localstack-core/localstack/services/lambda_/event_source_mapping/pollers/stream_poller.py
e,
)
raise CustomerInvocationError from e
elif "TrimmedDataAccessException" in str(e):
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you so much for adding this! 🙏 I'll tell the users next week about it for them to confirm to us it solves the issue and they don't need to run the command manually anymore! Thank you very much for going the extra mile here 🙇

@gregfurman gregfurman merged commit ab47589 into master Dec 13, 2024
31 checks passed
@gregfurman gregfurman deleted the fix/esm/dynamodb-no-shard branch December 13, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@joe4dev joe4dev Awaiting requested review from joe4dev

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

@dfangl dfangl Awaiting requested review from dfangl

Assignees

@gregfurman gregfurman

Labels
aws:dynamodbstreams AWS DynamoDB Streams semver: patch Non-breaking changes which can be included in patch releases type: bug Bug report
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@gregfurman @bentsku