Override minimum sequence number of deltamanager#20856
Conversation
github-actions
Bot
added
base: main
| } | ||
|
|
||
| export class DeltaManagerPendingOpsProxy extends BaseDeltaManagerProxy { | ||
| public get minimumSequenceNumber(): number { |
There was a problem hiding this comment.
This is only one method where MSN shows up. I think we need to review API and see what else needs to be faked.
For example, DM.lastMessage.minimumSequenceNumber should match what this thing returns (although it's worth pointing out that it will "magically" change its value once we reconnect, as MSN will move forward).
There was a problem hiding this comment.
I've updated this, do you think there are other parts? I did an investigation based on looking at minimumSequenceNumber and ISequenceDocumentMessage usage in the codebase.
There was a problem hiding this comment.
My investigation was based on two main paths - summarization and processing ops. Do you think the ScheduleManager needs to have this "proxied" view of the world?
github-actions
Bot
added
the
area: tests
| // It allows us to lie to the layers below so that they can maintain enough local state for rebasing ops. | ||
| if (useDeltaManagerOpsProxy) { | ||
| const pendingOpsDeltaManagerProxy = new DeltaManagerPendingOpsProxy( | ||
| summarizerDeltaManagerProxy, |
There was a problem hiding this comment.
nit: use outerDeltaManager instead of summarizerDeltaManagerProxy here. Allows easier recording of adapters (if needed)
| const { message, local } = messageWithContext; | ||
|
|
||
| // Intercept to reduce minimum sequence number to the delta manager's minimum sequence number. | ||
| messageWithContext.message.minimumSequenceNumber = this.deltaManager.minimumSequenceNumber; |
There was a problem hiding this comment.
How do we ensure that we have enough validation in place that everything works fine?
At minimum, I'd assert here that MSN moves (with this change) only backwards. I.e. we could replace it with smaller number only on message.
Ideally, we would have an assert somewhere that it's noop if we have no pending messages or already connected and went through resubmit flow.
| const minPendingSeqNum = this.pendingStateManager.minimumPendingMessageSequenceNumber; | ||
| if ( | ||
| minPendingSeqNum !== undefined && | ||
| minPendingSeqNum < this.deltaManager.minimumSequenceNumber |
There was a problem hiding this comment.
Can it happen that minPendingSeqNum > this.deltaManager.minimumSequenceNumber ?
I think not, and if it's the case - we should assert that.
There was a problem hiding this comment.
Definitely, adding a comment to explain a scenario for such an occurrence. Just submitting a regular op should do the trick, as this is based on ref seq number. Any other lagging client can cause min seq number to be lesser than expected.
| // D-C-AB | ||
| // E-HIJ-FG-D-C-AB | ||
| // ^----------^ | ||
| sharedString3.insertText(0, "AB"); |
There was a problem hiding this comment.
fwiw, I think this test may be sensitive to sequencing order of the ops in each segment, which is no longer deterministic due to the e2e nature (you can take pains to make it so, but I'm not sure).
I think we should probably lean more heavily on unit test coverage rather than e2e in this realm anyway, and I see you added some to pending state manager, so I'm not super concerned.
There was a problem hiding this comment.
Ok, I can see what I can do to fix this. You're right this can lead to test flakiness
There was a problem hiding this comment.
A PR CI hit this failure. I'll send you the result. This might need to be disabled.
There was a flakey test created by this, #20856 Since the test really isn't to test the service, and we require the service to behave in a certain way when it comes to ordering ops (the service does not need to promise a specific order, just that they are ordered), we will just use local server as we can predict the behavior quite predictably.
There is no task associated with this. Relevant reference PR: #20856 @agarwal-navin and I had an offline conversation about this comment, and it was hard to understand. I've now improved the comment for it to be easier to understand. --------- Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
5 participants
AB#7843
Override the minimumSequenceNumber of the
DeltaManagerfrom theContainerRuntimeto the layers below so that DDSes maintain the proper local state window so that they can properly apply and process local ops when rebasing or applying stashed ops.