8000 [SFN] Fix access to selected items in Distributed Map state by gregfurman · Pull Request #11861 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

[SFN] Fix access to selected items in Distributed Map state #11861

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 1 commit into from
Nov 18, 2024
Merged

[SFN] Fix access to selected items in Distributed Map state #11861

merged 1 commit into from
Nov 18, 2024

Conversation

gregfurman
Copy link
Contributor
@gregfurman gregfurman commented Nov 18, 2024 •
edited
Loading

Motivation

This PR fixes input passed to an ItemSelector not being correctly propogated to each iteration of a distributed map run.

Changes

  • Instead of reading input data from the stack, map runs will instead consume (via pop) from the stack.
  • Added ItemSelector tests for both a DISTRIBUTED and INLINE map type.

@gregfurman gregfurman self-assigned this Nov 18, 2024
@gregfurman gregfurman added aws:stepfunctions AWS Step Functions semver: patch Non-breaking changes which can be included in patch releases labels Nov 18, 2024
@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0    2 suites  ±0   30m 21s ⏱️ - 1h 13m 40s
1 056 tests  - 2 470  914 ✅  - 2 219  142 💤  - 251  0 ❌ ±0 
1 058 runs   - 2 470  914 ✅  - 2 219  144 💤  - 251  0 ❌ ±0 

Results for commit e5abd8c. ± Comparison against base commit 5b44747.

This pull request removes 2472 and adds 2 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.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_map_state_config_distributed_item_selector_parameters
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_map_state_item_selector_parameters

@gregfurman gregfurman marked this pull request as ready for review November 18, 2024 15:07
dominikschubert
dominikschubert approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member
@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch.

Kinda easy to miss. Would love if we had a bit more documentation around when to pop vs. peek the stack.

MEPalma
MEPalma approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor
@MEPalma MEPalma 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.

LGTM!
It's true, the map run should consume it's input instead of just reading it. Not doing this meant that the iteration logic could not evaluate on the state's input (or it's reduction through InputPath), but on the ItemsPath's result. Good catch, thank you @gregfurman!

@gregfurman gregfurman merged commit f419bd5 into master Nov 18, 2024
35 of 36 checks passed
@gregfurman gregfurman deleted the fix/sfn/distributed-map branch November 18, 2024 16:10
@gregfurman gregfurman added status: response required Waiting for a response from the reporter and removed status: response required Waiting for a response from the reporter labels Nov 19, 2024
@gregfurman gregfurman mentioned this pull request Nov 19, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikschubert dominikschubert dominikschubert approved these changes

@MEPalma MEPalma MEPalma approved these changes

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

Assignees

@gregfurman gregfurman

Labels
aws:stepfunctions AWS Step Functions semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gregfurman @dominikschubert @MEPalma
0