-
Notifications
You must be signed in to change notification settings - Fork 536
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
Improve out of order op processing #4749
Conversation
if (to !== undefined && this.lastQueuedSequenceNumber >= to) { | ||
// the client caught up while we were trying to fetch ops from storage | ||
// bail out since we no longer need to request these ops | ||
callback([]); |
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 do not think making an empty call is required, or we do it in other places.
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.
The callback invokes this.refreshDelayInfo(this.deltaStorageDelayId);
. If we skip this callback, that won't be called. Is that okay?
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 updated it to not pass anything so only refreshDelayInfo
is invoked.
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's important. I think it's cleaner for fetchMissingDeltas() to call it after it's done.
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 assume it still has to call refreshDelayInfo
in every callback too?
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.
Yes
⯅ @fluidframework/base-host: +267 Bytes
■ @fluid-example/bundle-size-tests: No change
Baseline commit: a252465 |
Consider the following scenario:
pending
and callfetchMissingDeltas
to ask for ops 151 - 159At this point the client has all the ops it needs to be fully connected. However
deltaManager
did not process op 160, which remains inpending
. So the client is stuck waiting forfetchMissingDeltas
to complete. This PR makes the client process that pending op so it won't get stuck.Changes:
catchUpCore
method withprocessPendingOps
method that only processes pending ops