-
Notifications
You must be signed in to change notification settings - Fork 219
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
Port pismo replay tool improvements #7432
Conversation
499472e
to
810b8fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Anything that lets this work is fine by me, so I didn't look too carefully at replay-transcript.js
itself, just the code in src/
to make sure production code isn't impacted.
The one guideline I'd suggest is to think of ways that replay-transcript.js
can export a function or two to let us run a unit test on it, just a basic smoketest that makes sure a simple transcript can be replayed without crashing. Not necessary to do now, but something to aim for in the near future.
* replayTranscript: (startPos: number | undefined) => Promise<number?>, | ||
* makeSnapshot?: (endPos: number, ss: SnapStore) => Promise<SnapshotResult>, | ||
* makeSnapshot?: undefined | ((endPos: number, ss: SnapStore) => Promise<SnapshotResult>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean to have both the ?
in makeSnapsot?
and the undefined |
?
The original intention was that manager.makeSnapshot
may or may not be present, but if it is, then it must be a function with the given signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why I had to do this, but I think tsc was complaining without it back in the original pismo work
I do remember something about makeSnapshot
being provided as argument and unconditionally installed in the returned object even if undefined
, so this is technically more accurate.
Store VC metadata syscalls results for handling missing from transcript Options to configure syscall mismatch
With different load points
Add keep worker at specific delivery nums
…orkers Fix concurrent snapshot save for custom snapStore
Do not attempt to rebuild
Force use custom snapstore if snapshot differences allowed
Unify mutex logic around worker snapshot load / save
810b8fd
to
779f188
Compare
refs: #6723
Description
This is a port of the replay tool changes only of #6723, adapted for the changes in master since the pismo fork.
The changes to the extract transcript tools needed more independent work and were extracted into a separate PR: #7434.
This does include some changes layered on top of the rebase, to make the replay tool working correctly with the new snapstore abstraction, and support the new bundle handling.
Changes compared to original PR
Diff of the ported commits vs original PR:
Security Considerations
None, this only changes internal tools.
Scaling Considerations
None
Documentation Considerations
Updates and fixes a few types
Testing Considerations
Manually verified that the transcripts generated by #7434 when running a modified
test-vaults-integration.js
could be replayed successfully.