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

Summarize one more time before closing #643

Closed
arinwt opened this issue Nov 22, 2019 · 4 comments
Closed

Summarize one more time before closing #643

arinwt opened this issue Nov 22, 2019 · 4 comments
Assignees
Labels
area: runtime Runtime related issues feature-request New feature or requests
Milestone

Comments

@arinwt
Copy link
Contributor

arinwt commented Nov 22, 2019

If parent container closes/disconnects, currently the summarizer container stops immediately.
It might be nice if the summarizer container instead sort of "queues" a stop, and tries to generate one last summary before stopping/closing.

This can be a perf improvement in the case that the last user of a document disconnects their main client (i.e. switching active documents without closing the browser tab), because it gives them a chance to summarize trailing ops.

It also opens up the possibility to summarize while troubleshooting/having connection issues because of server rejection. Currently the server will reject all ops incoming for a document that hasn't summarized for too long. It will disconnect the client trying to send these ops, so it will disconnect the main client without their summarizer a chance to run. This change will allow the summarizer to at least try to send 1 summary before closing. In troubleshooting scenarios, this summary might fix whatever issues were there.

@arinwt arinwt self-assigned this Nov 22, 2019
@curtisman curtisman added the feature-request New feature or requests label Jan 21, 2020
@arinwt
Copy link
Contributor Author

arinwt commented Jan 21, 2020

One drawback to this is the potential up-front perf cost. For example, in the app currently switching from one document to another would trigger this new final summarize on the old document, but at the same time it is trying to load a new document, which is already a critical perf point.

@curtisman curtisman added the area: runtime Runtime related issues label Jan 31, 2020
@curtisman curtisman added this to the Future milestone Feb 2, 2020
@arinwt
Copy link
Contributor Author

arinwt commented Apr 7, 2020

@vladsud

After thinking some more: change #1722 do not close on nack + summarize one last time after main client closes + delay spawn summarizer, when all 3 are combined, seems to mean that the main client needs to try to spawn a summarizer even if it is disconnected.

Without the change in #1722, do not retry on nack:

  1. Main client would start, eventually connect (see join op), and then get nacked and disconnected.
  2. Repeat (1) for 5 sec (spawn summarizer delay)
  3. Still repeating (1), once reconnected again: spawn summarizer client.
  4. Still repeating (1), once disconnected again: summarizer client should try to summarizer one last time (with new change) before it closes.

With this PR change, step (1) will not repeat, so the main client won't be connected after 5 sec delay, so it may not spawn a summarizer.

The change in #1722 seems useful on its own, so I was thinking about ways to still have all 3 changes.

  • One idea was if there is a way to detect if there is a huge count of outstanding ops up front (specifically, at load runtime), then we could reduce the delay spawn summarizer (to zero at some threshold, at least below server nack threshold). Then we can guarantee we at least try to spawn the summarizer.
  • Second idea is to bypass the spawn summarizer delay based on the difference between the first "connected" join message sequence number and the delta manager initialSequenceNumber.
  • Fallback option is to keep the delay, but requires the main client to try to spawn a summarizer client after the delay, even if it is disconnected. Then the summarizer client will be responsible for making sure there are no other summarizer clients, etc. instead of the previously more strict check that its parent client was elected summarizer by quorum join order. This will be slightly relaxing the restriction, but should be okay.

I am currently doing the second idea, but I'm wondering your thoughts; I don't see a way to achieve the first idea- is it possible to know how far behind we are when calling instantiateRuntime for the container runtime? If not, is there some other heuristic we can use to detect desperate need of a summary?

@vladsud
Copy link
Contributor

vladsud commented Apr 7, 2020

As I was reading through options, I also stopped on #2 as most viable.
I like it because

  • information is readily available (it's not for # 1, there is no easy to get it there).
  • I hope that we can avoid cases of multiple clients creating competing summarizers, or at least reduce it to very small probability.
    Looking at your draft...

@arinwt
Copy link
Contributor Author

arinwt commented Apr 14, 2020

Closed with PR #1729

Went with option (2) above. Changed to allow the spawned summarizer container to try to summarize one last time before closing after the main container disconnects. (if >50 ops since last successful summary).

@arinwt arinwt closed this as completed Apr 14, 2020
@danielroney danielroney modified the milestones: Future, May 2020 Oct 6, 2020
Josmithr added a commit that referenced this issue Jun 24, 2022
# Improve `@fluidframework/test-client-utils` API docs

The intention behind this PR is to improve API documentation on the consumer-facing APIs of the `@fluidframework/test-client-utils` package. It also includes some changes in transitively exposed APIs from packages upon which it depends. 

See here for the public-facing, generated docs: https://fluidframework.com/docs/apis/test-client-utils/

[ADO #643](https://dev.azure.com/fluidframework/internal/_workitems/edit/643)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues feature-request New feature or requests
Projects
None yet
Development

No branches or pull requests

4 participants