9480 make flushmode immutable #10503

kian-thompson · 2022-06-02T02:47:55Z

AB#393

For more information about how to contribute to this repo, visit this page

Description

The 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. See #9480.

PR Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
My code follows the code style of this project.
I ran the lint checks which produced no new errors nor warnings for my changes.
I have checked to ensure there aren't other open Pull Requests for the same update/change.

Does this introduce a breaking change?

Yes
No

Method setFlushMode was removed. Please remove all usage of this method and instead set the flush mode when constructing the container runtime object.

vladsud · 2022-06-03T22:22:03Z

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

@@ -1296,7 +1296,7 @@ export class ContainerRuntime extends TypedEventEmitter<IContainerRuntimeEvents>
                flushMode: () => this.flushMode,
                reSubmit: this.reSubmit.bind(this),
                rollback: this.rollback.bind(this),
-                setFlushMode: (mode) => this.setFlushMode(mode),
+                setFlushMode: (mode) => {},


I'm not sure why we need this callback at all - should it be removed from IRuntimeStateHandler? If it's required for testing purposes, I'd make it optional and not provide here.

This removal will be done as part of AB#394

vladsud · 2022-06-03T22:43:40Z

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

-        try {
-            this.trackOrderSequentiallyCalls(callback);
+        this.trackOrderSequentiallyCalls(callback);
+        if (this.flushMode === FlushMode.Immediate) {
            this.flush();


I'm not sure this code is correct. Previously, we always switched to FlushMode.TurnBased, and that ensured that ContainerRuntime.submit() would pass batch = true when calling submitMaybeChunkedMessages() (this._flushMode === FlushMode.TurnBased), which would instruct DeltaManager.submit() not to call flush() after every op.

With this change, I believe we will keep flushing ops after every op. Which is not what caller expecting.
If I got it right, it would be great to see if our UT captures this, and if not - what tests are missing to cover this case.

Plus, I believe you are breaking recursive orderSequentially() calls. flush() should be done only for outer most orderSequentially() call. Previously it was handled thanks change in flushMode and thus nested calls not calling flush(). Same feedback - we need to ensure that test coverage exists and captures this bug.

I think you can make it work (without changing all the time flushMode), but code in ContainerRuntime.submit() should take into account this._orderSequentiallyCalls when deciding if batch is on or not:

clientSequenceNumber = this.submitMaybeChunkedMessages( type, content, serializedContent, maxOpSize, this._flushMode === FlushMode.TurnBased || this._orderSequentiallyCalls > 0, opMetadataInternal);

I'd also probably not do flush() here, but rather move it to trackOrderSequentiallyCalls() and do two things (if possible):

Flush when this._orderSequentiallyCalls drops to zero (and this.flushMode === FlushMode.Immediate)

inline trackOrderSequentiallyCalls() into orderSequentially()

I'd simplify interface and remove batch argument at all, and fully rely on explicit flush() calls. This likely can be done, but requires care due to back-compat concerns - I'd definitely do so as a set of separate PRs.

This check also needs to be done when setting batch: true metadata to the op in submit.
IMO its better to switch to Turnbased in orderSequentially. The state machine then is simple - TurnBased means add batch metadata and don't flush. Immediate means don't add batch metadata and flush.
This will also make it future proof and we don't need to add this._orderSequentiallyCalls check everywhere.

msfluid-bot · 2022-06-07T21:21:23Z

⯅ @fluid-example/bundle-size-tests: +1.82 KB

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	394.67 KB	395.52 KB	⯅ +867 Bytes
connectionState.js	711 Bytes	711 Bytes	■ No change
containerRuntime.js	211.42 KB	212.26 KB	⯅ +866 Bytes
loader.js	146.19 KB	146.24 KB	⯅ +45 Bytes
map.js	38.29 KB	38.29 KB	■ No change
matrix.js	123 KB	123 KB	■ No change
odspDriver.js	145.95 KB	145.99 KB	⯅ +44 Bytes
odspPrefetchSnapshot.js	36.82 KB	36.87 KB	⯅ +44 Bytes
sharedString.js	140.74 KB	140.74 KB	■ No change
Total Size	1.24 MB	1.24 MB	⯅ +1.82 KB

Baseline commit: ab29527

Generated by 🚫 dangerJS against aa9e53f

kian-thompson · 2022-06-07T22:06:36Z

packages/test/test-end-to-end-tests/src/test/batching.spec.ts

        dataObject2 = await requestFluidObject<ITestFluidObject>(container2, "default");
        dataObject2map1 = await dataObject2.getSharedObject<SharedMap>(map1Id);
        dataObject2map2 = await dataObject2.getSharedObject<SharedMap>(map2Id);

+        // @ts-expect-error older versions rely on the "setFlushMode" method


@andre4i Since older versions rely on the setFlushMode method I had to do this sort of hacky workaround. Do you know of a better way, or should we delay this PR until your code is in N-2?

I think that because we're in next, the N-2 thing doesn't apply anymore in terms of breaking back-compat. Just make sure you don't forget about this and track the removal in the cleanup work item. You can also add a todo comment and mention the work item number.

vladsud

LGTM, but want to make sure that we do have test coverage for issue I pointed out earlier in #10503 (comment)

agarwal-navin · 2022-06-08T16:45:29Z

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

@@ -2755,7 +2715,7 @@ export class ContainerRuntime extends TypedEventEmitter<IContainerRuntimeEvents>
                content,
                serializedContent,
                maxOpSize,
-                this._flushMode === FlushMode.TurnBased,
+                this._flushMode === FlushMode.TurnBased || this._orderSequentiallyCalls > 0,


The code above where it adds batch: true to ops also needs to be updated.
It may be simpler to update the flushMode to TurnBased in orderSequentially if its Immediate (similar to what is happening today). We can still update it internally without exposing any APIs to do it.

agarwal-navin · 2022-06-08T16:53:39Z

packages/test/test-end-to-end-tests/src/test/batching.spec.ts

-            setupBatchMessageListener(dataObject1, dataObject1BatchMessages);
-            setupBatchMessageListener(dataObject2, dataObject2BatchMessages);
-        });
-
        describe("Flushing of batches via orderSequentially", () => {


These orderSequentially tests need to be done for FlushMode Immediate as well. Basically, even if the FlushMode is Immediate, these tests should work the same. Can you please add them?

agarwal-navin · 2022-06-08T16:55:25Z

packages/test/test-end-to-end-tests/src/test/batching.spec.ts

@@ -367,6 +380,10 @@ describeFullCompat("Flushing ops", (getTestObjectProvider) => {
        }

        describe("Automatic flushing of batches via orderSequentially", () => {


Similar to my comment above, these tests need to work for Immediate FlushMode as well.

agarwal-navin · 2022-06-08T17:05:10Z

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

@@ -103,9 +103,9 @@ describe("Runtime", () => {
                                        state: "disabled",
                                    },
                                },
+                                flushMode,


We should probably add tests to this file that validates container runtime correctly adds batch metadata and generates batch events in both flush modes.

kian-thompson · 2022-06-16T23:06:35Z

Closing this PR to combine with AB#394

# [AB#393](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/393) and [AB#394](https://dev.azure.com/fluidframework/internal/_workitems/edit/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](#10503 (comment)). - [AB#1060](https://dev.azure.com/fluidframework/internal/_workitems/edit/1060) - Deprecate `IContainerRuntime.flush`. This method should not be exposed publicly. - [AB#1059](https://dev.azure.com/fluidframework/internal/_workitems/edit/1059)

kian-thompson added 8 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

github-actions bot added area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc breaking change This PR or issue would introduce a breaking change public api change Changes to a public API base: next PRs targeted against next branch labels Jun 2, 2022

Fix lint

93c8858

vladsud reviewed Jun 3, 2022

View reviewed changes

kian-thompson added 4 commits June 6, 2022 16:12

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

5fc04e4

Fix recursive orderSequentially calls

93a3833

Fix linting and API changes

230ce3d

Fix batching tests

aa9e53f

kian-thompson marked this pull request as ready for review June 7, 2022 22:00

kian-thompson requested review from msfluid-bot and a team as code owners June 7, 2022 22:00

kian-thompson requested a review from andre4i June 7, 2022 22:03

kian-thompson commented Jun 7, 2022

View reviewed changes

vladsud approved these changes Jun 8, 2022

View reviewed changes

agarwal-navin reviewed Jun 8, 2022

View reviewed changes

kian-thompson marked this pull request as draft June 15, 2022 23:54

kian-thompson closed this Jun 16, 2022

kian-thompson mentioned this pull request Jun 30, 2022

Make flushMode immutable and remove all tracking of it #10930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

9480 make flushmode immutable #10503

9480 make flushmode immutable #10503

kian-thompson commented Jun 2, 2022 •

edited

Loading

vladsud Jun 3, 2022

kian-thompson Jun 6, 2022 •

edited

Loading

vladsud Jun 3, 2022

agarwal-navin Jun 8, 2022

msfluid-bot commented Jun 7, 2022

kian-thompson Jun 7, 2022

andre4i Jun 8, 2022

vladsud left a comment

agarwal-navin Jun 8, 2022

agarwal-navin Jun 8, 2022

agarwal-navin Jun 8, 2022

agarwal-navin Jun 8, 2022

kian-thompson commented Jun 16, 2022

		@@ -367,6 +380,10 @@ describeFullCompat("Flushing ops", (getTestObjectProvider) => {
		}

		describe("Automatic flushing of batches via orderSequentially", () => {

9480 make flushmode immutable #10503

9480 make flushmode immutable #10503

Conversation

kian-thompson commented Jun 2, 2022 • edited Loading

Description

PR Checklist

Does this introduce a breaking change?

Choose a reason for hiding this comment

kian-thompson Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfluid-bot commented Jun 7, 2022

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

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 Jun 16, 2022

kian-thompson commented Jun 2, 2022 •

edited

Loading

kian-thompson Jun 6, 2022 •

edited

Loading