Skip to content
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.

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

[SFN] Add support for previousEventId #9342

Merged
merged 8 commits into from Oct 19, 2023
Merged

[SFN] Add support for previousEventId #9342

merged 8 commits into from Oct 19, 2023

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Oct 12, 2023

Motivation

When dispatching GetExecutionHistory calls about some State Machine Execution, the current implementation would compile previousEventId values according to a global incremental counter. This logic means that in information about the true event source of any event is often incorrect, especially for distributed workflows such as Map and Parallel states. This PR aims at ensuring that for each event, the true source event id is provided.

Changes

Introduces a EventHistoryContext in Environment objects. These allow the interpreter to cache the last event id published. As each context object operates in its own evaluation scope, this is information is ensured to not be tampered by the global execution of the program. Values in the context can then be resynchronised with the parent scope during frame closing operations, or skipped through frame deletion operations (see MapRun workflows).
Evaluation logic for map iteration workers is modified to fit this new design, and resolved a number of concurrency issues when closing pool jobs or evaluation stops.
Related changes extend to other relevant state resolvers such as state_map and state_parallel.

Testing

  • removes all skip_snapshot_verify for previous_event_id
  • enables test_sfn_scenarios for v2

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Oct 12, 2023
@MEPalma MEPalma added this to the 3.0 milestone Oct 12, 2023
@MEPalma MEPalma self-assigned this Oct 12, 2023
@github-actions
Copy link

github-actions bot commented Oct 12, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 7m 26s ⏱️
2 259 tests 1 684 ✔️ 575 💤 0
2 260 runs  1 684 ✔️ 576 💤 0

Results for commit bffa43c.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Oct 16, 2023

Coverage Status

coverage: 82.927% (+0.07%) from 82.861% when pulling bffa43c on MEP-sfn-prev_id into ca43d39 on master.

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, thanks for jumping on this. 🚀

@markers.snapshot.skip_snapshot_verify(
paths=["$..loggingConfiguration", "$..tracingConfiguration", "$..previousEventId"]
)
@markers.snapshot.skip_snapshot_verify(paths=["$..loggingConfiguration", "$..tracingConfiguration"])
Copy link
Member

Choose a reason for hiding this comment

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

great to see that we can start removing some skipped verifications 🚀

@MEPalma MEPalma merged commit 61d5d07 into master Oct 19, 2023
27 checks passed
@MEPalma MEPalma deleted the MEP-sfn-prev_id branch October 19, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants