fix(Staging-Mode): Handle rollback case for DocumentSchemaMessages #24776
Conversation
github-actions
bot
added
base: main
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the rollback handling for DocumentSchema messages sent in staging mode by resetting the op pending flag instead of using the previous onDisconnect behavior. Key changes include:
- Updating tests to use pendingOpNotAcked instead of onDisconnect.
- Refactoring DocumentsSchemaController to use an opPending flag for controlling schema message generation.
- Adjusting ContainerRuntime logic to properly resubmit or rollback DocumentSchemaChange ops and improving related telemetry and summarizer client handling.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/container-runtime/src/test/documentSchema.spec.ts | Tests updated to call pendingOpNotAcked to reset pending op state. |
| packages/runtime/container-runtime/src/summary/documentSchema.ts | Replaces generateOp with opPending flag and updates related method logic. |
| packages/runtime/container-runtime/src/pendingStateManager.ts | Adds documentation for opMetadata, localOpMetadata, and sequenceNumber. |
| packages/runtime/container-runtime/src/gc/garbageCollection.ts | Enhances comments regarding telemetry and GC op behavior. |
| packages/runtime/container-runtime/src/containerRuntime.ts | Refactors summarizer client flag usage and op resubmission, including squashing logic. |
Comments suppressed due to low confidence (1)
packages/runtime/container-runtime/src/summary/documentSchema.ts:691
- Consider expanding the method documentation for pendingOpNotAcked to clarify that it resets the opPending flag for rollback operations in staging mode, differentiating it from the older onDisconnect behavior.
public pendingOpNotAcked(): void {
| if (!canSendOps) { | ||
| this.documentsSchemaController.onDisconnect(); | ||
| } |
There was a problem hiding this comment.
@vladsud - Can you confirm that this call was only needed in the case where we had a pending Document Schema op which failed to make it to the ordering service? i.e. the resubmit case? I moved this call (and renamed it) to resubmit, just wanted to make sure I don't also need it here for some reason I can't see yet.
cc @scottn12 and @ChumpChief as other people familiar with Document Schema
There was a problem hiding this comment.
I had to read the code to refresh my memory, but yes, your assumption looks correct.
markfields
requested review from
anthony-murphy,
ChumpChief,
Abe27342,
scottn12,
vladsud,
a team,
steffenloesch,
pragya91,
jason-ha,
jatgarg,
kian-thompson,
WillieHabi and
MarioJGMsoft
Description
A DocumentSchema message could be sent in Staging Mode*, which means we need to handle what happens if it's rolled back.
Turns out it's easy - no internal state changes when generating the op to be submitted, only when processing. Except for a boolean flag indicating whether we are waiting for a pending op, or if we should still try to generate one. So all that's needed is to reset this flag when rolling back.
* e.g. if the very first thing the user does on boot (in a session where a schema upgrade is needed) is to enter staging mode. Document Schema messages aren't submitted until another op is, so it would end up staged along with that op in this case.
Reviewer Guidance
This replaces #24729. This branch includes #24774 as well, so blocking this on merging that first.