-
Notifications
You must be signed in to change notification settings - Fork 537
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
Fetching ops from PUSH #6954
Fetching ops from PUSH #6954
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,8 @@ export class OdspDocumentService implements IDocumentService { | |
|
||
private _opsCache?: OpsCache; | ||
|
||
private currentConnection?: OdspDocumentDeltaConnection = undefined; | ||
vladsud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* @param odspResolvedUrl - resolved url identifying document that will be managed by this service instance. | ||
* @param getStorageToken - function that can provide the storage token. This is is also referred to as | ||
|
@@ -184,6 +186,9 @@ export class OdspDocumentService implements IDocumentService { | |
concurrency, | ||
async (from, to, telemetryProps) => service.get(from, to, telemetryProps), | ||
async (from, to) => { | ||
if (this.currentConnection !== undefined && !this.currentConnection.disposed) { | ||
this.currentConnection.requestOps(from, to); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be behind some kind of flag? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels pretty safe to me, with two caveats:
@GaryWilber - any thoughts on state of # 2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll have a killswitch on the push side to disable processing get_ops requests - it would make it a no-op. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @GaryWilber , do you have it implemented already? Or planning to add? This change will hit prod in a matter of 1-2 weeks after being approved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The killswitch is implemented but it's not deployed to production yet. ETA to full prod deployment would be 2 weeks from now (8/16) |
||
} | ||
const res = await this.opsCache?.get(from, to); | ||
return res as ISequencedDocumentMessage[] ?? []; | ||
}, | ||
|
@@ -247,6 +252,7 @@ export class OdspDocumentService implements IDocumentService { | |
connection.on("op", (documentId, ops: ISequencedDocumentMessage[]) => { | ||
this.opsReceived(ops); | ||
}); | ||
this.currentConnection = connection; | ||
return connection; | ||
} catch (error) { | ||
this.cache.sessionJoinCache.remove(this.joinSessionKey); | ||
|
@@ -294,7 +300,7 @@ export class OdspDocumentService implements IDocumentService { | |
io: SocketIOClientStatic, | ||
client: IClient, | ||
webSocketUrl: string, | ||
): Promise<IDocumentDeltaConnection> { | ||
): Promise<OdspDocumentDeltaConnection> { | ||
const startTime = performance.now(); | ||
const connection = await OdspDocumentDeltaConnection.create( | ||
tenantId, | ||
|
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.
Should the latency of the get_ops request be logged with this event? Could track the start time for the given nonce so it's accessible here.
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 make code a bit more complicated. I want to get this in first, and then keep improving based on feedback.
Including getting data on if this is the actual direction we want to go. I.e. I believe we will need to make a choice of either using this flow to understand better / easier why gaps are happening and keep improving PUSH, or vice verse - experiment with simplifying PUSH toward putting complex on client side