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

Deprecate enableGroupedBatching #23260

Merged

Conversation

MarioJGMsoft
Copy link
Contributor

@MarioJGMsoft MarioJGMsoft commented Dec 5, 2024

Description

After discussing the work in task #8124, we concluded that it was best to make Batch Grouping dependent on Batch Compression.

This is the first step where we are tagging enableGroupedBatching as deprecated in containerRuntime.ts

Acceptance Criteria:
containerRuntime.enableGroupedBatching is now tagged as deprecated

Execution Plan:
Create the changeset file explaining the change and add the @deprecated tag

Reviewer Guidance

Please let me know if there's anything I should change or make better.

Fixes AB#26356

@github-actions github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API base: main PRs targeted against main branch labels Dec 5, 2024
@github-actions github-actions bot removed public api change Changes to a public API area: tests Tests to add, test infrastructure improvements, etc labels Dec 6, 2024
@github-actions github-actions bot added the public api change Changes to a public API label Dec 6, 2024
@github-actions github-actions bot added changeset-present and removed area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct labels Dec 6, 2024
@MarioJGMsoft MarioJGMsoft changed the title Remove support for Batch Grouping configuration Deprecate enableGroupedBatching Dec 6, 2024
@MarioJGMsoft MarioJGMsoft marked this pull request as ready for review December 6, 2024 23:33
@MarioJGMsoft MarioJGMsoft requested review from a team as code owners December 6, 2024 23:33
@MarioJGMsoft MarioJGMsoft requested review from a team, pragya91, jatgarg, tyler-cai-microsoft, kian-thompson and rajatch-ff and removed request for a team December 6, 2024 23:33

Marked `IContainerRuntimeOptions.enableGroupedBatching` as deprecated

- We want to remove the ability to configure batch grouping, so `IContainerRuntimeOptions.enableGroupedBatching` is now being marked as deprecated and batch grouping will now depend on batch compression.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- We want to remove the ability to configure batch grouping, so `IContainerRuntimeOptions.enableGroupedBatching` is now being marked as deprecated and batch grouping will now depend on batch compression.
- We will remove the ability to disable Grouped Batching in v2.20.0. The only exception (i.e. where Grouped Batching would be disabled) is for compatibility with older (v1) clients, and this will be implemented without needing to expose `IContainerRuntimeOptions.enableGroupedBatching`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied changes

Copy link
Contributor

@jzaffiro jzaffiro left a comment

Choose a reason for hiding this comment

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

Changeset looks good, approving for docs!


Marked `IContainerRuntimeOptions.enableGroupedBatching` as deprecated

- We will remove the ability to disable Grouped Batching in v2.20.0. The only exception (i.e. where Grouped Batching would be disabled) is for compatibility with older (v1) clients, and this will be implemented without needing to expose `IContainerRuntimeOptions.enableGroupedBatching`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- We will remove the ability to disable Grouped Batching in v2.20.0. The only exception (i.e. where Grouped Batching would be disabled) is for compatibility with older (v1) clients, and this will be implemented without needing to expose `IContainerRuntimeOptions.enableGroupedBatching`.
We will remove the ability to disable Grouped Batching in v2.20.0. The only exception (i.e. where Grouped Batching would be disabled) is for compatibility with older (v1) clients, and this will be implemented without needing to expose `IContainerRuntimeOptions.enableGroupedBatching`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied changes

@@ -519,6 +519,7 @@ export interface IContainerRuntimeOptions {
* The grouping an ungrouping of such messages is handled by the "OpGroupingManager".
*
* By default, the feature is enabled.
* @deprecated The ability to configure Grouped Batching is now removed and it is now disabled if compression is disabled.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @deprecated The ability to configure Grouped Batching is now removed and it is now disabled if compression is disabled.
* @deprecated Grouped Batching can no longer be disabled, it is required for the proper functioning of the Fluid Framework

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't technically true. We can't change existing functionality in a deprecation as that's effectively a breaking change. This deprecation is instead notifying consumers that they soon won't be able to disable the feature, and that there is no alternative as "it is required for the proper functioning of the Fluid Framework".

Suggested change
* @deprecated The ability to configure Grouped Batching is now removed and it is now disabled if compression is disabled.
* @deprecated The ability to disable Grouped Batching is deprecated and will be removed in v2.20.0. This feature is required for the proper functioning of the Fluid Framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied changes

"section": deprecation
---

Marked `IContainerRuntimeOptions.enableGroupedBatching` as deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Marked `IContainerRuntimeOptions.enableGroupedBatching` as deprecated
IContainerRuntimeOptions.enableGroupedBatching is now deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if removing the inline code formatting was intentional. I personally do like to see it even in headers.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it was intentional. Opinions vary, but last time we discussed this the general consensus was that code formatting in headings is more distracting than helpful, especially when so much of every heading is code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied changes


Marked `IContainerRuntimeOptions.enableGroupedBatching` as deprecated

- We will remove the ability to disable Grouped Batching in v2.20.0. The only exception (i.e. where Grouped Batching would be disabled) is for compatibility with older (v1) clients, and this will be implemented without needing to expose `IContainerRuntimeOptions.enableGroupedBatching`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- We will remove the ability to disable Grouped Batching in v2.20.0. The only exception (i.e. where Grouped Batching would be disabled) is for compatibility with older (v1) clients, and this will be implemented without needing to expose `IContainerRuntimeOptions.enableGroupedBatching`.
The `IContainerRuntimeOptions.enableGroupedBatching` property is deprecated and will be removed in version 2.20.0. This will mean that the grouped batching feature can no longer be disabled. In versions 2.20.0 and beyond, grouped batching is required for the proper functioning of the Fluid Framework.
The sole case where grouped batching will be disabled is for compatibility with older v1 clients, and this will be implemented without any need for the configurable `IContainerRuntimeOptions.enableGroupedBatching` option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -519,6 +519,7 @@ export interface IContainerRuntimeOptions {
* The grouping an ungrouping of such messages is handled by the "OpGroupingManager".
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The grouping an ungrouping of such messages is handled by the "OpGroupingManager".
* The grouping and ungrouping of such messages is handled by the "OpGroupingManager".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied changes

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170006 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors


@MarioJGMsoft MarioJGMsoft merged commit 49d8e75 into microsoft:main Dec 11, 2024
32 checks passed
@MarioJGMsoft MarioJGMsoft deleted the removeBatchGroupingConfiguration branch December 11, 2024 21:46
Josmithr pushed a commit to Josmithr/FluidFramework that referenced this pull request Dec 12, 2024
## Description

After discussing the work in task
[microsoft#8124](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8124),
we concluded that it was best to make Batch Grouping dependent on Batch
Compression.

This is the first step where we are tagging enableGroupedBatching as
deprecated in containerRuntime.ts

Acceptance Criteria:
containerRuntime.enableGroupedBatching is now tagged as deprecated

Execution Plan: 
Create the changeset file explaining the change and add the @deprecated
tag

## Reviewer Guidance

Please let me know if there's anything I should change or make better.

Fixes
[AB#26356](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/26356)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants