-
Notifications
You must be signed in to change notification settings - Fork 562
Staging Mode: Integrate OrderSequentially with staging mode #24124
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
Staging Mode: Integrate OrderSequentially with staging mode #24124
Conversation
| configurations: { "Fluid.ContainerRuntime.EnableRollback": true }, | ||
| skip: [ | ||
| ...[0, 2, 7, 10, 13, 15, 26, 29, 39, 52, 61, 66, 67, 75, 76, 82, 84, 88, 90], // RollbackError: Unsupported op type for rollback (shared intervals) | ||
| ...[1, 3, 6, 8, 14, 23, 30, 53, 55, 56, 58, 59, 64, 70, 77, 80, 86, 93, 99], // RollbackError: Can't rollback attach message |
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.
RollbackError: Can't rollback attach message is gone, some shifted to other buckets
packages/test/local-server-stress-tests/src/test/localServerStress.spec.ts
Outdated
Show resolved
Hide resolved
| this.rollback(content, localOpMetadata), | ||
| ); | ||
| if (this.attachState === AttachState.Attached) { | ||
| this.updateDocumentDirtyState(this.pendingMessagesCount !== 0); |
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.
Yeah looking at callers to this, I think we could be more rigid and just tie it to PSM state. (not for this PR)
…y/FluidFramework-1 into sm/reset-dirystate
| if (this.dirtyContainer !== checkpointDirtyState) { | ||
| this.updateDocumentDirtyState(checkpointDirtyState); | ||
| } | ||
| stageControls?.discardChanges(); |
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.
This will call outbox.flush, but without the if check that is below around the this.flush call. (PS if we called this.flush here it would assert since the counter isn't back to 0 yet).
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.
it will only happen if the counter is zero as we only set the stage controls var when the counter is zero. all recursive calls will have undefined.
2789c28 to
538d66b
Compare
| * Licensed under the MIT License. | ||
| */ | ||
|
|
||
| export class RunCounter { |
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.
@markfields thinking i'll bring this to main. i like it better for orderSequentially and re-entrancy, as it reduces mutable state in the container runtime, and i think makes all the code easier to follow.
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.
This change integrates staging mode into OrderSequentially which adds support of attaches, and naively implements keeping the dirty state consistent within staging mode it self.
AB#34238