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

Draft: Change API of staging mode #23974

Draft
wants to merge 2 commits into
base: test/staging-mode
Choose a base branch
from
Draft

Draft: Change API of staging mode #23974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cdc304a
Switch to handle based api
anthony-murphy Mar 3, 2025
fa56816
generate docs
anthony-murphy Mar 4, 2025
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
56 changes: 42 additions & 14 deletions packages/runtime/container-runtime/src/containerRuntime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
IResponse,
ITelemetryBaseLogger,
} from "@fluidframework/core-interfaces";
import type { ErasedType } from "@fluidframework/core-interfaces";
import {
type IErrorBase,
IFluidHandleContext,
Expand All @@ -50,6 +51,7 @@ import {
LazyPromise,
PromiseCache,
delay,
unreachableCase,
} from "@fluidframework/core-utils/internal";
import {
IClientDetails,
Expand Down Expand Up @@ -98,7 +100,6 @@ import {
IInboundSignalMessage,
type IRuntimeMessagesContent,
type ISummarizerNodeWithGC,
type StageControls,
} from "@fluidframework/runtime-definitions/internal";
import {
GCDataBuilder,
Expand Down Expand Up @@ -3479,25 +3480,52 @@ export class ContainerRuntime
return result;
}

enterStagingMode = (): StageControls => {
const checkpoint = this.outbox.getBatchCheckpoints(true);
const branchInfo = {
discardChanges: () => {
private stagingModeHandle?: { checkpoint: ReturnType<Outbox["getBatchCheckpoints"]> };
public get inStagingMode(): boolean {
return this.stagingModeHandle !== undefined;
}
enterStagingMode = (): ErasedType<"StagingModeHandle"> => {
if (this.stagingModeHandle !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you're using this handle to approximate a mutex. I would agree that mutually exclusive ownership of the staging mode state is a requirement, but I'd also argue that if the customer is in a state where they've lost sight of who might turn staging mode on/off then they're in trouble anyway. Having a failable enterStagingMode() seems annoying to account for on the customer side too, invites weird patterns like waitForAndEnableStagingMode().

I think the better solution is to keep it consolidated at the container runtime level in FF, and if the customer wants to broaden its visibility then they have the option to dole out the capability in whatever way fits their need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have any thoughts on what that would look like? maybe create prototype, as i'm not sure how to do that is the abstract.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, here's an example. Description has a second variation too:
#23982

throw new Error("Already in staging mode");
}
this.stagingModeHandle = {
checkpoint: this.outbox.getBatchCheckpoints(true),
};
return this.stagingModeHandle as unknown as ErasedType<"StagingModeHandle">;
};

exitStagingMode = (
handle: ErasedType<"StagingModeHandle">,
arg: { type: "accept" } | { type: "reject" },
): void => {
if (this.stagingModeHandle === undefined) {
throw new Error("Must be in staging mode");
}
if ((this.stagingModeHandle as unknown) !== handle) {
throw new Error("Invalid StagingModeHandle");
}
const { checkpoint } = this.stagingModeHandle;
this.stagingModeHandle = undefined;
const { type } = arg;
switch (type) {
case "accept": {
checkpoint.unblockFlush();
this.outbox.flush();
return;
}
case "reject": {
assert(
checkpoint.blobAttachBatch.isEmpty() && checkpoint.idAllocationBatch.isEmpty(),
"other batches must be empty",
);

checkpoint.mainBatch.rollback();
checkpoint.unblockFlush();
},
commitChanges: () => {
checkpoint.unblockFlush();
this.outbox.flush();
},
};

return branchInfo;
return;
}
default: {
unreachableCase(type);
}
}
};

/**
Expand Down
2 changes: 2 additions & 0 deletions ...time/datastore-definitions/api-report/datastore-definitions.legacy.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ export interface IFluidDataStoreRuntime extends IEventProvider<IFluidDataStoreRu
// (undocumented)
readonly IFluidHandleContext: IFluidHandleContext;
// (undocumented)
get inStagingMode(): boolean;
// (undocumented)
readonly logger: ITelemetryBaseLogger;
// (undocumented)
readonly objectsRoutingContext: IFluidHandleContext;
Expand Down
2 changes: 2 additions & 0 deletions packages/runtime/datastore-definitions/src/dataStoreRuntime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ export interface IFluidDataStoreRuntime
*/
readonly attachState: AttachState;

get inStagingMode(): boolean;

readonly idCompressor: IIdCompressor | undefined;

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/runtime/datastore/api-report/datastore.legacy.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ export class FluidDataStoreRuntime extends TypedEventEmitter<IFluidDataStoreRunt
// (undocumented)
get IFluidHandleContext(): this;
// (undocumented)
get inStagingMode(): boolean;
// (undocumented)
get isAttached(): boolean;
// (undocumented)
get logger(): ITelemetryLoggerExt;
Expand Down
4 changes: 4 additions & 0 deletions packages/runtime/datastore/src/dataStoreRuntime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ export class FluidDataStoreRuntime
return this.dataStoreContext.connected;
}

get inStagingMode(): boolean {
return this.dataStoreContext.containerRuntime.inStagingMode;
}

public get clientId(): string | undefined {
return this.dataStoreContext.clientId;
}
Expand Down
17 changes: 10 additions & 7 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 @@ -76,7 +76,13 @@ export interface IContainerRuntimeBase extends IEventProvider<IContainerRuntimeB
// (undocumented)
readonly disposed: boolean;
// (undocumented)
readonly enterStagingMode: () => StageControls;
readonly enterStagingMode: () => StagingModeHandle;
// (undocumented)
readonly exitStagingMode: (handle: StagingModeHandle, arg: {
type: "accept";
} | {
type: "reject";
}) => void;
generateDocumentUniqueId(): number | string;
getAbsoluteUrl(relativeUrl: string): Promise<string | undefined>;
getAliasedDataStoreEntryPoint(alias: string): Promise<IFluidHandle<FluidObject> | undefined>;
Expand All @@ -86,6 +92,8 @@ export interface IContainerRuntimeBase extends IEventProvider<IContainerRuntimeB
snapshotTree: ISnapshotTree;
sequenceNumber: number;
}>;
// (undocumented)
get inStagingMode(): boolean;
orderSequentially(callback: () => void): void;
submitSignal: (type: string, content: unknown, targetClientId?: string) => void;
// (undocumented)
Expand Down Expand Up @@ -397,12 +405,7 @@ export interface OpAttributionKey {
}

// @alpha @sealed (undocumented)
export interface StageControls {
// (undocumented)
readonly commitChanges: () => void;
// (undocumented)
readonly discardChanges: () => void;
}
export type StagingModeHandle = ErasedType<"StagingModeHandle">;

// @alpha (undocumented)
export type SummarizeInternalFn = (fullTree: boolean, trackState: boolean, telemetryContext?: ITelemetryContext, incrementalSummaryContext?: IExperimentalIncrementalSummaryContext) => Promise<ISummarizeInternalResult>;
Expand Down
14 changes: 9 additions & 5 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 @@ -16,6 +16,7 @@ import type {
ITelemetryBaseLogger,
} from "@fluidframework/core-interfaces";
import type {
ErasedType,
IFluidHandleInternal,
IProvideFluidHandleContext,
} from "@fluidframework/core-interfaces/internal";
Expand Down Expand Up @@ -193,10 +194,7 @@ export interface IDataStore {
* @alpha
* @sealed
*/
export interface StageControls {
readonly commitChanges: () => void;
readonly discardChanges: () => void;
}
export type StagingModeHandle = ErasedType<"StagingModeHandle">;

/**
* A reduced set of functionality of IContainerRuntime that a data store context/data store runtime will need
Expand All @@ -218,7 +216,13 @@ export interface IContainerRuntimeBase extends IEventProvider<IContainerRuntimeB
*/
orderSequentially(callback: () => void): void;

readonly enterStagingMode: () => StageControls;
get inStagingMode(): boolean;
readonly enterStagingMode: () => StagingModeHandle;

readonly exitStagingMode: (
handle: StagingModeHandle,
arg: { type: "accept" } | { type: "reject" },
) => void;
/**
* Submits a container runtime level signal to be sent to other clients.
* @param type - Type of the signal.
Expand Down
2 changes: 1 addition & 1 deletion packages/runtime/runtime-definitions/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export type {
IFluidParentContext,
IFluidDataStoreContextDetached,
IPendingMessagesState,
StageControls,
StagingModeHandle,
} from "./dataStoreContext.js";
export { FlushMode, FlushModeExperimental, VisibilityState } from "./dataStoreContext.js";
export type { IProvideFluidDataStoreFactory } from "./dataStoreFactory.js";
Expand Down
2 changes: 2 additions & 0 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 @@ -442,6 +442,8 @@ export class MockFluidDataStoreRuntime extends EventEmitter implements IFluidDat
// (undocumented)
get IFluidHandleContext(): IFluidHandleContext;
// (undocumented)
readonly inStagingMode = false;
// (undocumented)
get isAttached(): boolean;
// (undocumented)
readonly loader: ILoader;
Expand Down
1 change: 1 addition & 0 deletions packages/runtime/test-runtime-utils/src/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,7 @@ export class MockFluidDataStoreRuntime
public get objectsRoutingContext(): IFluidHandleContext {
return this;
}
public readonly inStagingMode = false;

public readonly documentId: string = undefined as any;
public readonly id: string;
Expand Down
15 changes: 11 additions & 4 deletions packages/test/local-server-tests/src/test/stagingMode.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from "@fluidframework/container-loader/internal";
import { loadContainerRuntime } from "@fluidframework/container-runtime/internal";
import { type FluidObject } from "@fluidframework/core-interfaces/internal";
import type { StagingModeHandle } from "@fluidframework/runtime-definitions/internal";
import {
LocalDeltaConnectionServer,
type ILocalDeltaConnectionServer,
Expand Down Expand Up @@ -53,6 +54,12 @@ class RootDataObject extends DataObject {
public enterStagingMode() {
return this.context.containerRuntime.enterStagingMode();
}
public exitStagingMode(handle: StagingModeHandle, accept: boolean) {
return this.context.containerRuntime.exitStagingMode(
handle,
accept ? { type: "accept" } : { type: "reject" },
);
}
}

/**
Expand Down Expand Up @@ -175,7 +182,7 @@ describe("Scenario Test", () => {
const deltaConnectionServer = LocalDeltaConnectionServer.create();
const clients = await createClients(deltaConnectionServer);

const branchData = clients.original.dataObject.enterStagingMode();
const stagingModeHandle = clients.original.dataObject.enterStagingMode();
assert.deepStrictEqual(
clients.original.dataObject.state,
clients.loaded.dataObject.state,
Expand Down Expand Up @@ -210,7 +217,7 @@ describe("Scenario Test", () => {
"Expected mainline change to reach branch",
);

branchData.commitChanges();
clients.original.dataObject.exitStagingMode(stagingModeHandle, true);

await waitForSave(clients);

Expand All @@ -225,7 +232,7 @@ describe("Scenario Test", () => {
const deltaConnectionServer = LocalDeltaConnectionServer.create();
const clients = await createClients(deltaConnectionServer);

const branchData = clients.original.dataObject.enterStagingMode();
const stagingModeHandle = clients.original.dataObject.enterStagingMode();
assert.deepStrictEqual(
clients.original.dataObject.state,
clients.loaded.dataObject.state,
Expand Down Expand Up @@ -260,7 +267,7 @@ describe("Scenario Test", () => {
"states should match after save",
);

branchData.discardChanges();
clients.original.dataObject.exitStagingMode(stagingModeHandle, false);

await waitForSave(clients);

Expand Down
Loading