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.

Sign up for GitHub

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

Remove back-compat code related to the op reentry checks #22842

Merged
merged 12 commits into from
Nov 14, 2024
Merged

Remove back-compat code related to the op reentry checks #22842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8e256ba
Removing ensureNoDataModelChanges
MarioJGMsoft Oct 17, 2024
d036753
Fixed back-compat breaks
MarioJGMsoft Oct 17, 2024
c3698df
updated api runtime
MarioJGMsoft Oct 17, 2024
4de92ce
marked as deprecated based on comments
MarioJGMsoft Nov 11, 2024
817aefa
fixed merge
MarioJGMsoft Nov 12, 2024
6e52198
Merge branch 'main' into ensureNoDataModelChangers
MarioJGMsoft Nov 12, 2024
310f952
Added changeset and updated code comments
MarioJGMsoft Nov 12, 2024
352b303
Applied changes based on comments
MarioJGMsoft Nov 13, 2024
bffeadd
Applied changes based on comments
MarioJGMsoft Nov 13, 2024
c22885b
Merge branch 'main' into ensureNoDataModelChangers
MarioJGMsoft Nov 13, 2024
1598800
Applied changes based on comments
MarioJGMsoft Nov 13, 2024
5a45ae4
Applied changes based on comments
MarioJGMsoft Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/icy-webs-help.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
"@fluidframework/container-runtime": minor
"@fluidframework/runtime-definitions": minor
"@fluidframework/test-runtime-utils": minor
---
---
"section": legacy
---

"Remove `IFluidParentContext.ensureNoDataModelChanges` and its implementations

- `IFluidParentContext.ensureNoDataModelChanges` has been removed. [prior deprecation commit](https://github.com/microsoft/FluidFramework/commit/c9d156264bdfa211a3075bdf29cde442ecea234c)

- `MockFluidDataStoreContext.ensureNoDataModelChanges` has also been removed.
4 changes: 0 additions & 4 deletions packages/runtime/container-runtime/src/channelCollection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,6 @@ export function wrapContext(context: IFluidParentContext): IFluidParentContext {
getAudience: (...args) => {
return context.getAudience(...args);
},
// back-compat, to be removed in 2.0
ensureNoDataModelChanges: (...args) => {
jason-ha marked this conversation as resolved.
Show resolved Hide resolved
return context.ensureNoDataModelChanges(...args);
},
submitMessage: (...args) => {
return context.submitMessage(...args);
},
Expand Down
6 changes: 0 additions & 6 deletions packages/runtime/container-runtime/src/dataStoreContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,6 @@ export abstract class FluidDataStoreContext
public get containerRuntime(): IContainerRuntimeBase {
return this._containerRuntime;
}

// back-compat, to be removed in 2.0
public ensureNoDataModelChanges<T>(callback: () => T): T {
jason-ha marked this conversation as resolved.
Show resolved Hide resolved
return this.parentContext.ensureNoDataModelChanges(callback);
}

public get isLoaded(): boolean {
return this.loaded;
}
Expand Down
2 changes: 0 additions & 2 deletions .../runtime/runtime-definitions/api-report/runtime-definitions.legacy.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,6 @@ export interface IFluidParentContext extends IProvideFluidHandleContext, Partial
deleteChildSummarizerNode(id: string): void;
// (undocumented)
readonly deltaManager: IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>;
// @deprecated
ensureNoDataModelChanges<T>(callback: () => T): T;
// @deprecated (undocumented)
readonly gcThrowOnTombstoneUsage: boolean;
// @deprecated (undocumented)
Expand Down
13 changes: 0 additions & 13 deletions packages/runtime/runtime-definitions/src/dataStoreContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,19 +489,6 @@ export interface IFluidParentContext
*/
getAudience(): IAudience;

/**
* Invokes the given callback and expects that no ops are submitted
* until execution finishes. If an op is submitted, an error will be raised.
*
* Can be disabled by feature gate `Fluid.ContainerRuntime.DisableOpReentryCheck`
*
* @param callback - the callback to be invoked
*
* @deprecated
* // back-compat: to be removed in 2.0
*/
ensureNoDataModelChanges<T>(callback: () => T): T;

/**
* Submits the message to be sent to other clients.
* @param type - Type of the message.
Expand Down
2 changes: 0 additions & 2 deletions ...es/runtime/test-runtime-utils/api-report/test-runtime-utils.legacy.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,6 @@ export class MockFluidDataStoreContext implements IFluidDataStoreContext {
// (undocumented)
deltaManager: IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>;
// (undocumented)
ensureNoDataModelChanges<T>(callback: () => T): T;
Comment on lines 305 to -306
Copy link
Contributor

Choose a reason for hiding this comment

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

This technically should have been marked as deprecated before now. To be completely strict we would not remove this from the mock until properly deprecated. This is test-runtime-utils and maybe there is a different contract there. I will let API CR make final call.

Copy link
Contributor

@Josmithr Josmithr Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unless there is a reason we have to remove this in this PR and we are confident that none of our partners are using it (beyond just Loop), then I think it would be best to stick to the process and retain this as deprecated for future removal.

I see that we had an internal-facing comment noting it would be removed in 2.0, but our customers would have had no awareness of this, so I think it's worth erring on the side of caution.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, it seems like "ensureNoDataModelChanges" was deprecated on the interface and now he is removing it from the interface and classes.
Is it wrong to do so? In order to remove it, should it be marked as deprecated on interfaces and all classes implementing that interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jason-ha and @Josmithr Please take a look at this comment when you get time. We need to complete this breaking change PR by next week. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, missed the earlier responses. Yes, strictly speaking any implementation of the interface that declares the same properties should also mark the members as deprecated. There is nothing stopping MockFluidDataStoreContext from continuing to expose a ensureNoDataModelChanges member even if the interface it extends plans to remove it from the interface. So, to be explicit, any implementations for which the member is deprecated should be marked so as well.

Unless there is an urgent need to remove this, it would probably be better to mark the appropriate implementation members as @deprecated and add them to the issue tracking @legacy breaks we plan to make in 2.20.

I don't think there is anything stopping us from removing the deprecated interface member, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be okay making an exception for this one since the expected use is for mock to be used in places that would use ensureNoDataModelChanges via interface. Technically, we should have deprecated before and should wait, but I would be surprised if there is fallout.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Josmithr, are you also okay with this exception? Latest version has this removed per @jatgarg's suggestion to go the exception route.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MarioJGMsoft, please make sure you have tested a build with these changes using the FF+Loop integration process. Reply with the build results link. Thanks!
Reference docs: https://eng.ms/docs/experiences-devices/opg/office-shared/fluid-framework/fluid-framework-internal/fluid-framework/docs/dev/monitoring/ff-loop-testing

Copy link
Contributor

Choose a reason for hiding this comment

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

@Josmithr, are you also okay with this exception? Latest version has this removed per @jatgarg's suggestion to go the exception route.

I don't have a strong opinion either way. Introducing a break to something with "Mock" in the name seems safer than most alternatives, so I don't see any harm in it. But I do think we should generally try to be consistent with this stuff. 🤷‍♂️

// (undocumented)
readonly existing: boolean;
// (undocumented)
readonly gcThrowOnTombstoneUsage = false;
Expand Down
5 changes: 0 additions & 5 deletions packages/runtime/test-runtime-utils/src/mocksDataStoreContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,6 @@ export class MockFluidDataStoreContext implements IFluidDataStoreContext {
throw new Error("Method not implemented.");
}

// back-compat: to be removed in 2.0
public ensureNoDataModelChanges<T>(callback: () => T): T {
return callback();
}

public getQuorum(): IQuorumClients {
return undefined as any as IQuorumClients;
}
Expand Down
Loading