Make flushMode immutable and remove all tracking of it #10930

kian-thompson · 2022-06-30T21:28:59Z

AB#393 and AB#394

Description

The setFlushMode API is not currently used and enables a set of scenarios which should not be supported. Maintaining this API is costly, and it adds a non-trivial amount of complexity to the adjacent logic.

Because the flushMode should no longer change, the PendingStateManager should not track the switches in the previous system. Instead, the PendingStateManager will track and replay batches explicitly rather than through the setting of flush modes.

See #9480 and #9481.

Does this introduce a breaking change?

The setFlushMode has been removed from IContainerRuntimeBase. Please remove all usage of this method as FlushMode is now an immutable property for the container runtime, optionally provided at creation time via the IContainerRuntimeOptions interface.

The PendingStateManager no longer tracks flush mode as it is now an immutable property on the IContainerRuntimeBase. Instead, tracking of batches is done explicitly and they are replayed directly through usage of the orderSequentially method on IContainerRuntimeBase.

Other information or known dependencies

TODO after this PR:

Add tests to improve batch coverage in containerRuntime.spec.ts. See comment.
- AB#1060
Deprecate IContainerRuntime.flush. This method should not be exposed publicly.
- AB#1059

- Note: this doesn't seem to be related to any changes that I made, so something might be wrong with my local environment pulling protocol-definitions

BREAKING.md

vladsud · 2022-07-01T20:35:30Z

packages/runtime/container-runtime/src/containerRuntime.ts

-    }
-
-    public flush(): void {
+    public flush(isImmediateBatch: boolean = false): void {


Is it public only because pending state manager needs to call it? Can we make it private and provide it through delegation of some sort?

Also it would be great to have small comments on when this API is supposed to be called and what it means for this workflow.

flush has been deprecated and will be removed in AB#1076

vladsud · 2022-07-01T20:36:46Z

packages/runtime/container-runtime/src/containerRuntime.ts

@@ -2071,6 +2025,10 @@ export class ContainerRuntime extends TypedEventEmitter<IContainerRuntimeEvents>
        } finally {
            this._orderSequentiallyCalls--;
        }
+
+        if (this.flushMode === FlushMode.Immediate && this._orderSequentiallyCalls === 0) {


Isn't it !this.currentlyBatching() ?

I initially had it as !this.currentlyBatching() but thought explicitly showing the condition is much clearer

vladsud · 2022-07-01T20:39:19Z

packages/runtime/container-runtime/src/pendingStateManager.ts

+         * If the FlushMode is Immediate, we don't need to track an explicit flush call because every message is
+         * automatically flushed. So, flush is a no-op.
+         */
+        if (!isImmediateBatch && this.flushMode === FlushMode.Immediate) {


Can this check be moved out to container runtime layer? It feels like this layer (if possible) should just track batches and ops and not care about flush modes. I.e. it's responsibility of container runtime layer to convert its API surface to a stream of batches and ops (illumining flattening nested orderSequentially calls)

Maybe it's for another set of refactoring :)

vladsud

FYI - I only briefly looked through the code

andre4i

Just some minor comments.

Some of the code deserves some sort of state transition diagram attached to it, but that's a comment for a lot of areas in there.

Great job, BTW. This is awesome 👍 👍

BREAKING.md

andre4i · 2022-07-19T22:14:02Z

packages/runtime/container-runtime/src/containerRuntime.ts

+    /**
+     * Flush the pending ops manually.
+     * This method is expected to be called at the end of a batch.
+     * @param isImmediateBatch - is this "flush" being called at the end of a "FlushMode.Immedite" batch?


Suggested change

* @param isImmediateBatch - is this "flush" being called at the end of a "FlushMode.Immedite" batch?

* @param isImmediateBatch - is this "flush" being called at the end of a "FlushMode.Immediate" batch?

Do we need to expose this publicly? If we're the only ones to be calling flush with isImmediateBatch === true maybe flush with parameter can be private and the public flush will always be with false? Just trying to see if we can decrease the surface area of the API even more.

I think it's fine for now since flush has been deprecated. I'll revisit this in AB#1076

packages/runtime/container-runtime/src/containerRuntime.ts

packages/test/local-server-tests/src/test/opsOnReconnect.spec.ts

kian-thompson · 2022-07-25T16:39:31Z

@tylerbutler Is next still protected?
Edit: I was linked the thread in Teams. Tell me if my understanding is incorrect. We use the tag to have the bot perform the merge because users could accidentally squash merge which will cause bad merge conflicts when merging to/from main.

ghost · 2022-07-25T18:51:51Z

Hello @kian-thompson!

Because this pull request has the msftbot: merge-next label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

[AB#1107](https://dev.azure.com/fluidframework/internal/_workitems/edit/1107) For more information about how to contribute to this repo, visit [this page](https://github.com/microsoft/FluidFramework/blob/main/CONTRIBUTING.md). ## Description The `PendingStateManager` should just track batches and ops and not care about the `flushMode` of the container runtime. See #10930 (comment).

kian-thompson added 30 commits May 25, 2022 10:17

Remove setFlushMode

e13174d

Fix some unit tests

162dd5a

Merge branch 'next' into 9480-make-flushmode-immutable

2da0010

Fix batching tests

32f4eee

Mark breaking change

e75f8b3

Fix flush mode validation tests

5e3ff94

Remore method from API md

263a5ba

Fix some back compat

0b1478e

Fix lint

93c8858

Merge branch 'next' into 9480-make-flushmode-immutable

5fc04e4

Fix recursive orderSequentially calls

93a3833

Fix linting and API changes

230ce3d

Fix batching tests

aa9e53f

Some fixes

c393969

Some work on removing flushMode tracking/changes

5dce59a

Implement batch tracking in PendingStateManager

776a93a

Fix build

0cc4c6b

Fix some unit tests

de12f84

Remove onlys

98b4e38

Update comment

b8754c4

Merge branch 'next' into 9480-make-flushmode-immutable

262aa70

Fix some back compat

cebf90e

Merge branch 'next' into 9480-make-flushmode-immutable

62986bd

Merge branch 'next' into 9480-make-flushmode-immutable

2b919e6

Fix some back-compat

f8595e4

- Note: this doesn't seem to be related to any changes that I made, so something might be wrong with my local environment pulling protocol-definitions

Cleanup some comments and expressions

3409fc6

Back compat

20a0407

Update API.md

175a25e

Add more batching unit tests

834ddef

Merge branch 'next' into 9480-make-flushmode-immutable

b173a2d

kian-thompson requested review from a team as code owners June 30, 2022 21:55

kian-thompson requested review from andre4i, agarwal-navin and vladsud June 30, 2022 21:55

vladsud reviewed Jul 1, 2022

View reviewed changes

BREAKING.md Outdated Show resolved Hide resolved

vladsud reviewed Jul 1, 2022

View reviewed changes

BREAKING.md Outdated Show resolved Hide resolved

vladsud reviewed Jul 1, 2022

View reviewed changes

vladsud approved these changes Jul 1, 2022

View reviewed changes

kian-thompson added 4 commits July 5, 2022 15:17

BREAKING.md changes and documentation for flush method

bd2b26d

Merge branch 'next' into 9480-make-flushmode-immutable

08d3a03

Fix build

b0475a3

Undo protocol-definitions modification

df7ccff

andre4i approved these changes Jul 19, 2022

View reviewed changes

kian-thompson added 3 commits July 22, 2022 22:25

Merge branch 'next' into 9480-make-flushmode-immutable

5217c10

Andrei codereview changes

c4607f5

Remove unused flushTrigger

99b170d

kian-thompson added the msftbot: merge-next label Jul 25, 2022

kian-thompson removed the msftbot: merge-next label Jul 25, 2022

Merge branch 'next' into 9480-make-flushmode-immutable

264f089

kian-thompson added the msftbot: merge-next label Jul 25, 2022

ghost merged commit 0ce3dd0 into microsoft:next Jul 25, 2022

kian-thompson deleted the 9480-make-flushmode-immutable branch July 25, 2022 20:06

kian-thompson mentioned this pull request Aug 3, 2022

Remove flushMode tracking from PendingStateManager #11419

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make flushMode immutable and remove all tracking of it #10930

Make flushMode immutable and remove all tracking of it #10930

kian-thompson commented Jun 30, 2022 •

edited

Loading

vladsud Jul 1, 2022

vladsud Jul 1, 2022

kian-thompson Jul 5, 2022

vladsud Jul 1, 2022

kian-thompson Jul 1, 2022

vladsud Jul 1, 2022

vladsud Jul 1, 2022

kian-thompson Jul 5, 2022

vladsud left a comment

andre4i left a comment

andre4i Jul 19, 2022

andre4i Jul 19, 2022

kian-thompson Jul 22, 2022

kian-thompson commented Jul 25, 2022 •

edited

Loading

ghost commented Jul 25, 2022

	* @param isImmediateBatch - is this "flush" being called at the end of a "FlushMode.Immedite" batch?
	* @param isImmediateBatch - is this "flush" being called at the end of a "FlushMode.Immediate" batch?

Make flushMode immutable and remove all tracking of it #10930

Make flushMode immutable and remove all tracking of it #10930

Conversation

kian-thompson commented Jun 30, 2022 • edited Loading

AB#393 and AB#394

Description

Does this introduce a breaking change?

Other information or known dependencies

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladsud left a comment

Choose a reason for hiding this comment

andre4i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kian-thompson commented Jul 25, 2022 • edited Loading

ghost commented Jul 25, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

kian-thompson commented Jun 30, 2022 •

edited

Loading

kian-thompson commented Jul 25, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.