-
Notifications
You must be signed in to change notification settings - Fork 522
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
Reduce excess summaries [0.9] #47
Conversation
Does client cope fine if we do not pass that config? I'd rather not have it here, as it's not useful anyway (for replay scenarios) Refers to: packages/drivers/replay-socket-storage/src/replayDocumentDeltaConnection.ts:199 in c1fc75c. [](commit_id = c1fc75c, deletion_comment = False) |
if (this.summaryPending) { | ||
const pendingTime = Date.now() - this.lastSummaryTime; | ||
if (pendingTime > this.configuration.maxAckWaitTime) { | ||
debug("Summarizer timed out while waiting for SummaryAck/SummaryNack from server."); |
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.
debug [](start = 20, length = 5)
Please replace debug() with logging error
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.
serviceConfiguration: {
Does client cope fine if we do not pass that config? I'd rather not have it here, as it's not useful anyway (for replay scenarios)
Refers to: packages/drivers/replay-socket-storage/src/replayDocumentDeltaConnection.ts:199 in c1fc75c. [](commit_id = c1fc75c, deletion_comment = False)
At least the summarizer should work fine. It merges the default summary config with whatever (if anything) is provided.
@@ -105,6 +121,14 @@ export class Summarizer implements IComponentLoadable, ISummarizer { | |||
return this.runDeferred.promise; | |||
} | |||
|
|||
private handleSummaryOp(op: ISequencedDocumentMessage) { | |||
if (op.type === MessageType.SummaryAck || op.type === MessageType.SummaryNack) { |
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.
if (op.type === MessageType.SummaryAck || op.type === MessageType.SummaryNack) { [](start = 8, length = 80)
It would be great if we could double check that these are matching summary message (i.e. same handle / seq number).
And log an error (into telemetry) when it's not the case
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 agree, but I'd like to do this in a larger fix into master.
@@ -116,32 +140,33 @@ export class Summarizer implements IComponentLoadable, ISummarizer { | |||
const summarySequenceNumber = this.lastOp ? this.lastOp.sequenceNumber : 1; | |||
|
|||
await this.generateSummary(); | |||
this.summaryPending = true; |
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.summaryPending = true; [](start = 12, length = 27)
Should this be above prior line? #Resolved
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.
Why? Only after the generateSummary() call is the summary actually pending server response. Anyway, the summarizing will still be true with a small overlap with summaryPending, so it still won't be able to slip in another summary. #Resolved
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 moved it before just to be safe, and added the assert in summarize().
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 fix makes the summarizer wait for SummaryAck/SummaryNack before summarizing again, with a new summary configuration: maxAckWaitTime (default to 10 minutes). After that it will resume summarizing.
Includes some unit tests to verify expected summary timing.