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.

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

Make flushMode immutable for a container. Deprecate ContainerRuntime.setFlushMode() #9480

Closed
agarwal-navin opened this issue Mar 14, 2022 · 4 comments
Assignees
Labels
ado Issue is tracked in the internal Azure DevOps backlog api deprecation Changes to a deprecated API triage
Milestone

Comments

@agarwal-navin
Copy link
Contributor

agarwal-navin commented Mar 14, 2022

From the conversation in #9432, we should make flushMode immutable.

Primary reason for this work is to enable / make it easier to do

@andre4i
Copy link
Contributor

andre4i commented Mar 15, 2022

To add:

  • flushmode may be set via IContainerRuntimeOptions
  • must remove the IContainerRuntimeBase.setFlushMode() function
  • must follow the guidelines at https://github.com/microsoft/FluidFramework/wiki/API-Deprecation
  • must ensure whether or not someone is using actually using this API (add a telemetry event when this property is toggled) - my guess is that the feature is not currently used
  • must simplify all other code paths which assume flushmode changes during the lifetime of the runtime (separate workitem for this, as the work is slightly more involved and orthogonal to the API deprecation).

@andre4i andre4i changed the title Make flushMode immutable for a container Make flushMode immutable for a container. Deprecate ContainerRuntime.setFlushMode() Mar 15, 2022
@andre4i andre4i added the api deprecation Changes to a deprecated API label Mar 15, 2022
@vladsud vladsud added this to the April 2022 milestone Mar 30, 2022
@andre4i
Copy link
Contributor

andre4i commented Mar 31, 2022

Deprecated API

IContainerRuntimeBase.setFlushMode(mode: FlushMode): void;

Context

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.

Approach

FlushMode can be set at runtime creation, using IContainerRuntimeOptions and will remain immutable for the lifetime of the object.

Workarounds for the existing scenarios:

  • Runtime has FlushMode.TurnBased and the client needs ad-hoc behavior similar to FlushMode.Immediate? Force a JS turn after each operation (use a construct such as Promise.resolve().then)
  • Runtime has FlushMode.Immediate and the client needs ad-hoc behavior similar to FlushMode.TurnBased? Use IContainerRuntimeBase.orderSequentially

Dependencies
n/a

Compatibility Concerns
n/a

Phases

  • Mark the API as deprecated and emit a telemetry event for its use
  • Remove the API and all it's usages

Expected Timeline

  • 0.60

Open Questions

TBD

@andre4i
Copy link
Contributor

andre4i commented Apr 1, 2022

Confirmed the API is not currently used in Bohemia and Whiteboard.

@andre4i andre4i modified the milestones: April 2022, May 2022 Apr 6, 2022
@vladsud vladsud modified the milestones: May 2022, June 2022 May 20, 2022
@ghost ghost added the triage label May 20, 2022
@kian-thompson
Copy link
Contributor

kian-thompson commented May 25, 2022

Closed to track in AB#393

@curtisman curtisman added the ado Issue is tracked in the internal Azure DevOps backlog label Jun 15, 2022
ghost pushed a commit that referenced this issue Jul 25, 2022
# [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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ado Issue is tracked in the internal Azure DevOps backlog api deprecation Changes to a deprecated API triage
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

5 participants