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

One last summary before closing [0.15] #1729

Merged
merged 12 commits into from
Apr 13, 2020

Conversation

arinwt
Copy link
Contributor

@arinwt arinwt commented Apr 7, 2020

fixes #643

Part 1: SummaryManager eliminates initial delay before spawning summarizer if client join message op seq is much greater than deltaManager.initialSequenceNumber (4k ops right now, to ensure below 5k server limit of ODSP).

Part 2: Summarizer added async waitStop function to RunningSummarizer and await that before closing the entire container when the Summarizer gets stopped. New flow is:

  1. Main client disconnects, so its SummaryManager calls Summarizer.stop
  2. Summarizer.stop raises an event and completes the runCore deferred (essentially)
  3. Completing runCore, if connected: awaits runningSummarizer.waitStop and then closes the container
  4. Then everything is disposed

This should work with the other flow when the summarizer client disconnects first:

  1. Summarizer client disconnects, so the runCore deferred completes (essentially)
  2. Completing runCore disposes everything (since the container is not connected).

Also with the prevention of re-entrancy in Summarizer stop, it should work with the fallback validation in generateSummary.

This required a small relaxation of the fallback Summarizer generateSummary check that its parent client is still the elected summarizer client. Now it also allows that the itself is the elected summarizer, i.e. the only client on the document (because others would be elected summarizer).

Additional work:

  • Fix missing clientType info during request Container creation (already attached) by setting originalRequest in constructor so it is set first before calling get client() @vladsud @jatgarg - can you guys verify this part? (commit: 6995ef0 in this PR)
    edit: after resolving conflicts, it looks like this was already resolved in the same way :)
  • Prevent SummaryManager from spawning a new summarizer if there is any summarizer already in the quorum. This keeps other interactive clients from trying to spawn a summarizer if there is a lingering one trying to summarize one last time.
  • Since we don't wait for summary ack before closing the summarizer, make sure not to throw errors on timeout timer cancels in summarize.

@vladsud
Copy link
Contributor

vladsud commented Apr 7, 2020

        return;

Should this code be reorganized a bit?
Like can we here return this.summarizing.promise, and code in waitStop() just call trySummarizeCore() ?


Refers to: packages/runtime/container-runtime/src/summarizer.ts:453 in c216621. [](commit_id = c216621, deletion_comment = False)

Copy link
Contributor

@vladsud vladsud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@arinwt
Copy link
Contributor Author

arinwt commented Apr 7, 2020

        return;

I think I understand what you're suggesting...
Perhaps as it is now, it's a bit hacky, but this.summarizing.promise resolves sooner than trySummarizeCore(), so the code might end up getting a little messy.


In reply to: 610468953 [](ancestors = 610468953)


Refers to: packages/runtime/container-runtime/src/summarizer.ts:453 in c216621. [](commit_id = c216621, deletion_comment = False)

@arinwt arinwt marked this pull request as ready for review April 12, 2020 01:46
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

Successfully merging this pull request may close these issues.

2 participants