Skip to content
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

Sanity check creation of summarizer client around disconnected states. #107

Closed
vladsud opened this issue Sep 21, 2019 · 3 comments
Closed
Assignees

Comments

@vladsud
Copy link
Contributor

vladsud commented Sep 21, 2019

Any chance you can see if we deal with reconnections properly in summarize?

  1. Do we destroy summarizing container on lost connection?
    Note - createSummarizer() has "fluid-reconnect": false, which seems correct, assuming the rest works
  2. Do we always recreate summarizer on same client on reconnect if there were (and there are) no other clients connected?

Any chance that we can add telemetry (erorr) when there is no summurizer in a document and given client is connected?

Thanks!

@arinwt
Copy link
Contributor

arinwt commented Oct 1, 2019

Related to #198. This issue will be for adding additional telemetry and analysis.

We should consider calculating the number of simultaneous summarizers on a document based on documentId, clientId, and timestamps of Summarizer events, because it might be more objective and still useful. I have added clientId to summarizer events in PR #208.

@arinwt
Copy link
Contributor

arinwt commented Oct 3, 2019

Answering original questions with more information:

  1. Do we destroy summarizing container on lost connection?
    a) When the parent container disconnects, it will now stop the summarizer which will close its container. (PR Stop summarizer when parent container disconnects #208 addresses issue Multiple clients summarize at the same time #198)
    b) When the summarizer itself disconnects, it will resolve its run promise, which should essentially kill the container since reconnect is false, and its creator (SummaryManager from parent container) will no longer reference it.
  2. Do we always recreate summarizer on same client on reconnect if there were (and there are) no other clients connected?
    a) Any time the quorum addMember event fires, all clients recompute the summarizer. This should generally relaunch the summarizer if it has stopped.
    b) If only the summarizer itself disconnects, then I believe the parent will not restart the summarizer, because it will believe that it already has a summarizer running. This needs to be resolved.

My idea for resolving 2.b is to clean up the SummaryManager logic for detecting if it is already summarizing, and add code to try to recompute the summarizer if the summarizer run promise is fulfilled (indicating summarizer container disconnected).

The reason 2.b is a special case, is because if the parent container also disconnects/reconnects, it will "force" compute summarizer even if the thing it uses to track whether it is already summarizing or not indicates that it already has a summarizer. This is correct under the current conditions, because it only does this "force" on connect, which means that it was previously disconnected, and it won't start a summarizer when disconnected, and will stop the summarizer on disconnect.

@arinwt
Copy link
Contributor

arinwt commented Oct 23, 2019

Also PR #362 is related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants