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

feat(async-flow): startGeneration: a new synthetic LogEntry #10293

Merged
merged 1 commit into from
Oct 20, 2024

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Oct 17, 2024

closes: #9384

Description

This pull request introduces a startGeneration log entry to capture the generation (restart) number for subsequent log entries. The most important changes are grouped below by theme.

Log Entry Enhancements:

  • Introduced startGeneration and updated LogEntryShape to include a new log entry type for starting a generation (packages/async-flow/src/type-guards.js).

Log Store Improvements:

  • Retrieve the current generation number from a zone.mapStore and use it in LogStore ephemera to track its generation (packages/async-flow/src/log-store.js).
  • Modified prepareLogStore to define a predicate for filtering log entries, and added methods for retrieving unfiltered log entries (packages/async-flow/src/log-store.js).
  • Updated test cases to validate the new generation number and unfiltered log entries (packages/async-flow/test/log-store.test.js).

Replay Membrane Adjustments:

  • Added support for startGeneration in the replay membrane to handle generation-specific operations (packages/async-flow/src/replay-membrane.js).

Type Definitions:

  • Updated LogEntry and FutureLogEntry types to include the new generation entry type (packages/async-flow/src/types.ts).

Security Considerations

n/a

Scaling Considerations

Low overhead implementation.

Documentation Considerations

Not necessary yet, just laying groundwork for the infrastructure so that a future change can use it to implement asyncFlow versioning.

Testing Considerations

Some new *Unfiltered methods of the LogStore now have some tests.

Upgrade Considerations

Designed to enable future upgrade of async flows.

@michaelfig michaelfig self-assigned this Oct 17, 2024
@michaelfig michaelfig requested review from mhofman and erights October 17, 2024 18:29
Copy link

cloudflare-workers-and-pages bot commented Oct 17, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9b3a3d1
Status: ✅  Deploy successful!
Preview URL: https://53341ddd.agoric-sdk.pages.dev
Branch Preview URL: https://mfig-9384-log-store-generati.agoric-sdk.pages.dev

View logs

@michaelfig michaelfig force-pushed the mfig-9384-log-store-generation branch from a6d7cf1 to 21051a7 Compare October 17, 2024 19:35
@michaelfig michaelfig marked this pull request as ready for review October 17, 2024 20:46
@michaelfig michaelfig requested a review from a team as a code owner October 17, 2024 20:46
@michaelfig michaelfig force-pushed the mfig-9384-log-store-generation branch from 21051a7 to 4c21b73 Compare October 17, 2024 20:47
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
packages/async-flow/src/log-store.js Show resolved Hide resolved
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
packages/async-flow/src/replay-membrane.js Outdated Show resolved Hide resolved
packages/async-flow/src/type-guards.js Outdated Show resolved Hide resolved
packages/async-flow/src/type-guards.js Outdated Show resolved Hide resolved
@erights erights self-requested a review October 18, 2024 20:53
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
@erights erights self-requested a review October 19, 2024 22:11
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

I think I'd like to see a test of both a log reset and a flow restart that shows the generation of an invocation getting bumped when inserting new log entries. This likely can be done using explicit reset/restarts, and without a simulated liveslots upgrade (especially because I haven't landed the liveslots test framework improvements yet, and we don't have testing hooks to peek into the state of async flow, which would possibly be covered by #9753)

packages/async-flow/src/async-flow.js Outdated Show resolved Hide resolved
packages/async-flow/src/type-guards.js Outdated Show resolved Hide resolved
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
packages/async-flow/src/log-store.js Outdated Show resolved Hide resolved
@michaelfig michaelfig requested a review from mhofman October 20, 2024 00:50
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

All my issues are dealt with well, so approving to remove my blockage. But you should still iterate with @mhofman to his satisfaction before merging.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

LGTM (once the test snapshots are updated to reflect the new 0 based generation)

@mhofman
Copy link
Member

mhofman commented Oct 20, 2024

I am wondering if we should include a lastGenerationPush or something like that in the state of the log store, and some assertion that the start generation never regresses (it can stay the same).

@michaelfig
Copy link
Member Author

I am wondering if we should include a lastGenerationPush or something like that in the state of the log store, and some assertion that the start generation never regresses (it can stay the same).

I appreciate the need to have guarantees like this, but adding extra code for validation didn't strike me as the best thing to do. Ruminating on it, I'd prefer to encapsulate the generation number within prepareLogStore (where its increment-only behaviour would be easily verifiable by inspection) rather than passing it in from outside.

@michaelfig michaelfig marked this pull request as draft October 20, 2024 20:30
@michaelfig michaelfig marked this pull request as ready for review October 20, 2024 20:37
@michaelfig michaelfig force-pushed the mfig-9384-log-store-generation branch from 6caed9d to 9b3a3d1 Compare October 20, 2024 20:40
@mhofman
Copy link
Member

mhofman commented Oct 20, 2024

I'd prefer to encapsulate the generation number within prepareLogStore (where its increment-only behaviour would be easily verifiable by inspection) rather than passing it in from outside.

Yeah I think that's what was making me uneasy, that async-flow could mistakenly not pass the right generation. I prefer it container like this much better.

@michaelfig michaelfig added automerge:no-update (expert!) Automatically merge without updates asyncFlow related to membrane-based replay and upgrade of async functions labels Oct 20, 2024
@mergify mergify bot merged commit 7b2cca2 into master Oct 20, 2024
94 checks passed
@mergify mergify bot deleted the mfig-9384-log-store-generation branch October 20, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asyncFlow related to membrane-based replay and upgrade of async functions automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async-flow: need incarnation number in the log classifying entries
3 participants