Add compat mode option to AzureClient and TinyliciousClient #20997

ChumpChief · 2024-05-06T20:16:12Z

Based on @vladsud 's #20283 , this is a draft of API surface to toggle 1.x vs. 2.x compat mode.

Note that this feature's goal is to ensure compatible upgrade from 1.x to 2.x because the default settings don't allow for that currently. It is not a generic feature flagging API; feature owners must develop a go-to-market plan for their individual features. This API is intended to be for 1->2 only, and not for future version upgrades.

Putting up as a draft PR as a preview of the surface while I experiment with the usage. ~~Re: defaults and optionality, I optimized for (in order):~~

~~Folks upgrading from 1.x get a compatible experience by default~~
~~Default values prefer 2.x except where required to satisfy bullet 1~~
~~Easing eventual removal of the option by preferring optional over required so customers can omit the argument as much as possible except where required to satisfy bullet 1.~~

EDIT: From feedback, changing to mandatory params.

msfluid-bot · 2024-05-06T23:52:46Z

⯅ @fluid-example/bundle-size-tests: +424 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	453.11 KB	453.11 KB	■ No change
azureClient.js	550.36 KB	550.58 KB	⯅ +218 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	256.97 KB	256.97 KB	■ No change
fluidFramework.js	357 KB	357 KB	■ No change
loader.js	132.91 KB	132.91 KB	■ No change
map.js	41.43 KB	41.43 KB	■ No change
matrix.js	143.66 KB	143.66 KB	■ No change
odspClient.js	518.9 KB	519.1 KB	⯅ +206 Bytes
odspDriver.js	97.3 KB	97.3 KB	■ No change
odspPrefetchSnapshot.js	42.16 KB	42.16 KB	■ No change
sharedString.js	160.18 KB	160.18 KB	■ No change
sharedTree.js	356.98 KB	356.98 KB	■ No change
Total Size	3.19 MB	3.19 MB	⯅ +424 Bytes

Baseline commit: b47f044

Generated by 🚫 dangerJS against 522b9ee

vladsud · 2024-05-07T00:17:44Z

packages/framework/fluid-static/api-report/fluid-static.api.md

@@ -15,6 +15,9 @@ import { IFluidLoadable } from '@fluidframework/core-interfaces';
 import { IRuntimeFactory } from '@fluidframework/container-definitions/internal';
 import { ISharedObjectKind } from '@fluidframework/shared-object-base';

+// @public
+export type CompatMode = "1" | "2";


I'd make it an object / interface, not a string.
We will need to expose various options (like compression knobs) as our customers (like Autodesk) will ask for it.

There is no better place to do it other than here, on an object that is unique to a compat mode.
In other words, it should not be possible (should be compile error) for users to attempt to set or change compression settings for "1" schema as it does not apply / does not exist at that level of compatibility.

Same in reverse - when/if we remove ability some of these knobs in the future (let's say in 3.0 compat mode), ideally it should be a compile break for users do change these knobs on config that no longer allows that.

I also think it's more readable if it says { compatLevel: "1" } over "1" (even though more verbose).

My intention is to disallow nuanced configuration here - in the sync we agreed this feature is purely about migrating from 1->2, not for configuring other accessory features. Those features should design and establish their own appropriate API surface to enable/disable/customize.

E.g. GC uses the MonitoringContext to enable, or summaryCompression is offered on AzureClientProps. There might be better options available for how to do it and we can certainly discuss, but it's a separate scope from the primary concern of safely getting customers from 1->2.

I think these are not great examples (there is no overlap with compat settings).
Let's play the game in reverse - let's assume there is already compression setting out there and you are adding this PR. What would you do if user specifies compression and V1? Would it throw UsageError()? Let it use compression despite user saying - use compatibility mode?
At extreme, we will have all options in two places - one controlled from here, and one controlled from their respective knobs. Is this good API to have?

Given significantly complicated compat story and upgrade paths, I generally do like the idea to simplify and constrain in this case. Simplify: if the features are tightly coupled, offer them through unified APIs. Constrain: User should have very limited and explicit means to toggle/config anything related to our incompatible features.

vladsud · 2024-05-07T04:59:12Z

packages/framework/fluid-static/src/rootDataObject.ts

+			minimumBatchSizeInBytes: Number.POSITIVE_INFINITY, // disabled
+			compressionAlgorithm: CompressionAlgorithms.lz4,
+		},
+		// TODO: Include explicit disables for things that are currently off-by-default?


FYI - I merged #20604 that impacts choices here.
Yes, I'd explicitly mention all the settings here, as they should not change over time (though it's hard to enforce that for future settings that are not known at this time).

Ideally, we have tests that help us finding non-compat settings in the future, and thus force adding new things here (for example, such test should break once #20604 is picked up).

Might be useful reference point: filterRuntimeOptionsForVersion()

packages/framework/fluid-static/api-report/fluid-static.api.md

packages/framework/fluid-static/src/types.ts

Josmithr · 2024-05-09T18:08:21Z

packages/framework/fluid-static/src/types.ts

+ * Valid compat modes that may be specified when creating a DOProviderContainerRuntimeFactory.
+ * @public
+ */
+export type CompatMode = "1" | "2";


Is the intent to hide away details about the differences between the versions? If so, the type union makes sense to me. If not, an enum-like object would give us a place to document implications of each version.

That pattern would also discourage the use of magic constants in consuming code.

Yes for hiding the details - this will ultimately be paired with migration guidance documentation so providing a simple and straightforward set of instructions is a high-order goal. I'll experiment with other options here though (probably pending getting some of that guidance written so we can see how it looks).

scottn12 · 2024-05-10T14:43:22Z

packages/framework/fluid-static/src/rootDataObject.ts

 }

+const compatModeRuntimeOptions: Record<CompatMode, IContainerRuntimeOptions> = {


Do you think the number of features we need to disable for 1.X/enable for 2.X will continue to grow as 2.X development continues? If so, I'm wondering if it makes sense to give this object its own file if we will be continuously adding to it

I think that would represent dangerous changes, since if some featureFoo is added in say 2.3 and is incompatible with 1.4, then it's probably also incompatible with 2.0 and it would be a semver break to turn it on in a minor release.

If there are any items that get added to this config I think they'd most likely have to be features already in 2.0 but off-by-default, getting turned on in 2.3 (such that 2.0 won't blow up when it sees them).

That said, seems reasonable to split it to a separate file - I've done that in latest.

scottn12 · 2024-05-10T14:50:58Z

packages/service-clients/azure-client/api-report/azure-client.api.md

@@ -21,16 +22,16 @@ import { ScopeType } from '@fluidframework/protocol-definitions';
 // @public
 export class AzureClient {
    constructor(properties: AzureClientProps);
-    createContainer<const TContainerSchema extends ContainerSchema>(containerSchema: TContainerSchema): Promise<{
+    createContainer<const TContainerSchema extends ContainerSchema>(containerSchema: TContainerSchema, compatMode: CompatMode): Promise<{


Couple questions here -

This is a breaking change, right? Are we planning to add a changeset?

Did we consider making compatMode an optional param and having it default to either "1" or "2"? I'm wondering if we could avoid breaking changes.

I think @ChumpChief's callout was that we want devs to explicitly opt-in to learn about implications? So our API docs better be good :)

Yes, I'll add a changeset.

My initial PR had a default for the param but I received feedback from the team that an explicit decision was preferred.

scottn12 · 2024-05-10T17:49:00Z

packages/service-clients/azure-client/src/AzureClient.ts

Can we move the logic at line 66 (copied below) to compatModeRuntimeOptions in rootDataObject.ts? If we can't move configProvider configs to that file, maybe we should consider putting all the configurations in their own file like I suggested above. I think it would be best to have all of these configs in the same place

/** * Feature gates required to support runtime compatibility when V1 and V2 clients are collaborating */ const azureClientV1CompatFeatureGates = { // Disable Garbage Collection "Fluid.GarbageCollection.RunSweep": false, // To prevent the GC op "Fluid.GarbageCollection.DisableAutoRecovery": true, // To prevent the GC op "Fluid.GarbageCollection.ThrowOnTombstoneLoadOverride": false, // For a consistent story of "GC is disabled" };

Good callout - I believe the plan is to leave GC off in 2.x for AzureClient in all configs so I don't think this needs to participate in the toggling, but I see the appeal of consolidating v1-compat-motivated configs together. I think for initial checkin I'll omit this but will think about it.

packages/service-clients/azure-client/src/AzureClient.ts

sashasimic · 2024-05-13T17:02:37Z

packages/framework/fluid-static/api-report/fluid-static.api.md

@@ -15,6 +15,9 @@ import { IFluidLoadable } from '@fluidframework/core-interfaces';
 import { IRuntimeFactory } from '@fluidframework/container-definitions/internal';
 import { ISharedObjectKind } from '@fluidframework/shared-object-base';

+// @public
+export type CompatMode = "1" | "2";


Maybe a long term question, but the purpose here is to call out Supported Compat Modes for given release version? When we release 3.x I would assume compat mode options become 2 and 3?

I personally don't think this is a sustainable approach long term so I hope we have a better solution for 3? But if not, then yes this would become "2" | "3" in our 3.0 release to reflect that it only supports N - 1 interop.

ChumpChief · 2024-05-20T19:27:38Z

Marking this ready for review - I had a chance to experiment with this and it worked as expected in a variety of scenarios.

Main downside is that when attempting to load a "2"-mode document in a true 1.x client, the errors are a little obtuse (e.g. I hit 0x122 and "Summary metadata mismatch", Vlad's tests imply 0x162 and 0x121 would also be expected). I think this is probably best served by ensuring the documentation covers the situation well.

packages/framework/fluid-static/src/types.ts

Josmithr · 2024-05-21T16:55:29Z

packages/service-clients/tinylicious-client/.eslintrc.cjs

+			// Overrides for tests
+			files: ["src/test/*.spec.ts"],
+			rules: {
+				// https://mochajs.org/#arrow-functions


Can we add a comment explaining the rationale for disabling this for tests?

The URL in the comment (https://mochajs.org/#arrow-functions) describes the rationale - not sure if you saw that or if you're looking for more text directly in this file?

Oh, I assumed the link was just for more general context on the rule; I didn't look. I think we should add an explicit note here as a comment (fine if we want to say "see this link for rationale for disabling in tests" or something). I'd just like for any rule deviations to have clear rationale in the docs so devs know if/when they can be changed.

Added a note in latest

scottn12

LGTM! I think we will want to add more tests, but that can be done as a follow up. We are also still missing a changeset, but you mentioned you plan on adding that before merging

ChumpChief added 8 commits May 6, 2024 12:24

Add compat mode to createDOProviderContainerRuntimeFactory

86efaa8

Add API to AzureClient

d1e4589

Add compatMode to TinyliciousClient

afcff18

Change defaults and optionality

e043938

Omit compatMode from external-controller

80a6f07

Revert unintended md change

b27b0d5

Remove unnecessary disable

696a547

Add TODO

9ebcb23

github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: runtime Runtime related issues public api change Changes to a public API labels May 6, 2024

ChumpChief requested review from scottn12, vladsud and sashasimic May 6, 2024 20:16

github-actions bot added the base: main PRs targeted against main branch label May 6, 2024

Make compat mode mandatory argument, drop .x

8dce5ed

github-actions bot added the area: examples Changes that focus on our examples label May 6, 2024

ChumpChief added 3 commits May 6, 2024 14:46

Run tests for both modes

ac4c597

Switch to non-arrow functions for tests, skip ST for v1

2bd985a

Fix other tests

e519270

github-actions bot added the area: dev experience Improving the experience of devs building on top of fluid label May 6, 2024

ChumpChief added 2 commits May 6, 2024 15:50

Remove leftover comment

49a9990

Missed one

41119b4

vladsud reviewed May 7, 2024

View reviewed changes

Prettier fix

5d05bd4

vladsud reviewed May 7, 2024

View reviewed changes

ChumpChief added 2 commits May 7, 2024 09:19

Group batching is now on by default

a519ab5

Merge remote-tracking branch 'upstream/main' into CompatModeAPI

0526378

Josmithr reviewed May 9, 2024

View reviewed changes

packages/framework/fluid-static/api-report/fluid-static.api.md Outdated Show resolved Hide resolved

Josmithr reviewed May 9, 2024

View reviewed changes

packages/framework/fluid-static/src/types.ts Outdated Show resolved Hide resolved

Josmithr reviewed May 9, 2024

View reviewed changes

scottn12 reviewed May 10, 2024

View reviewed changes

sashasimic reviewed May 13, 2024

View reviewed changes

packages/service-clients/azure-client/src/AzureClient.ts Outdated Show resolved Hide resolved

sashasimic reviewed May 13, 2024

View reviewed changes

ChumpChief added 5 commits May 17, 2024 15:29

Merge remote-tracking branch 'upstream/main' into CompatModeAPI

9975cc9

Correct cross-version testing

ed3c44a

compat -> compatibility

671f6df

Fix api.md

d636013

Separate file

198d050

ChumpChief marked this pull request as ready for review May 20, 2024 19:23

Test loading 1.x -> 2.x in both 1 and 2 mode

b5acad1

Josmithr reviewed May 21, 2024

View reviewed changes

packages/framework/fluid-static/src/types.ts Show resolved Hide resolved

Josmithr reviewed May 21, 2024

View reviewed changes

Add more lint rule rationale

044a89b

scottn12 approved these changes May 21, 2024

View reviewed changes

ChumpChief added 2 commits May 21, 2024 13:39

Merge remote-tracking branch 'upstream/main' into CompatModeAPI

52860a0

Add changeset

522b9ee

ChumpChief merged commit 2730787 into microsoft:main May 21, 2024
30 checks passed

ChumpChief deleted the CompatModeAPI branch May 21, 2024 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compat mode option to AzureClient and TinyliciousClient #20997

Add compat mode option to AzureClient and TinyliciousClient #20997

ChumpChief commented May 6, 2024 •

edited by azure-boards bot

msfluid-bot commented May 6, 2024 •

edited

vladsud May 7, 2024

ChumpChief May 7, 2024

vladsud May 7, 2024 •

edited

sashasimic May 13, 2024

vladsud May 7, 2024

Josmithr May 9, 2024

Josmithr May 9, 2024

ChumpChief May 9, 2024

scottn12 May 10, 2024

ChumpChief May 20, 2024

scottn12 May 10, 2024

sashasimic May 13, 2024

ChumpChief May 20, 2024

scottn12 May 10, 2024

ChumpChief May 20, 2024

sashasimic May 13, 2024

ChumpChief May 20, 2024

ChumpChief commented May 20, 2024

Josmithr May 21, 2024

ChumpChief May 21, 2024

Josmithr May 21, 2024

ChumpChief May 21, 2024

scottn12 left a comment

		}

		const compatModeRuntimeOptions: Record<CompatMode, IContainerRuntimeOptions> = {

Add compat mode option to AzureClient and TinyliciousClient #20997

Add compat mode option to AzureClient and TinyliciousClient #20997

Conversation

ChumpChief commented May 6, 2024 • edited by azure-boards bot

msfluid-bot commented May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladsud May 7, 2024 • edited

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

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

ChumpChief commented May 20, 2024

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

scottn12 left a comment

Choose a reason for hiding this comment

ChumpChief commented May 6, 2024 •

edited by azure-boards bot

msfluid-bot commented May 6, 2024 •

edited

vladsud May 7, 2024 •

edited