[BREAKING IN 2.20] Added assert for compression ON grouping OFF #23608

MarioJGMsoft · 2025-01-17T23:23:05Z

Description

We no longer want to make it possible to create a container that has grouping off and compression on, so we will throw an error if we detect that it has been created that way.

Acceptance Criteria

A container throws an error when it is configured with compression on and grouping off.

Execution Plan

Add code to container runtime
Update related tests to make sure that they expect an error to be thrown.

Reviewer Guidance

Please let me know if there's something that I'm missing or that I should know about. Thanks!

Fixes: AB#28625

Some places tried to use undefined to disable. This does not work, it gets replaced with default values.

packages/runtime/container-runtime/src/containerRuntime.ts

markfields · 2025-01-20T17:39:52Z

@dannimad -- Marking this as release-blocking. The removal of this deprecated option was reverted, but we still need to make this breaking behavior change in 2.20. (cc @jason-ha as FYI)

cc @jzaffiro for docs review. Thanks!

packages/test/test-version-utils/src/compatUtils.ts

markfields · 2025-01-20T20:51:52Z

packages/runtime/container-runtime/src/test/containerRuntime.spec.ts

-					minimumBatchSizeInBytes: 614400,
-					compressionAlgorithm: CompressionAlgorithms.lz4,
-				},
+				compressionOptions: disabledCompressionConfig,


expand down to see enableGroupedBatching: false

markfields · 2025-01-20T20:53:18Z

packages/test/test-end-to-end-tests/src/test/blobs.spec.ts

@@ -231,7 +231,7 @@ describeCompat("blobs", "FullCompat", (getTestObjectProvider, apis) => {
 		]);
 	});

-	[false, true].forEach((enableGroupedBatching) => {
+	[true].forEach((enableGroupedBatching) => {


We should revisit the test matrix here after this merges - may want to ensure coverage with grouping and compression both disabled?

markfields · 2025-01-20T20:54:51Z

packages/test/test-end-to-end-tests/src/test/messageSize.spec.ts

@@ -257,7 +255,7 @@ describeCompat("Message size", "NoCompat", (getTestObjectProvider, apis) => {

 	const chunkingBatchesTimeoutMs = 200000;

-	[false, true].forEach((enableGroupedBatching) => {
+	[true].forEach((enableGroupedBatching) => {


Same as above, for future - what about both disabled?

jzaffiro

Approved for docs!

markfields · 2025-01-21T21:09:09Z

packages/test/test-end-to-end-tests/src/test/containerRuntime.spec.ts

@@ -120,6 +120,7 @@ describeCompat(
 						minimumBatchSizeInBytes: compression ? 1000 : Infinity,
 						compressionAlgorithm: CompressionAlgorithms.lz4,
 					},
+					enableGroupedBatching: compression, // Compression w/o grouping is not supported


@MarioJGMsoft I think we need to revisit this too - I think there's an existing gap in test coverage where "Grouping Disabled" isn't covered

jason-ha · 2025-01-21T21:38:48Z

@dannimad -- Marking this as release-blocking. The removal of this deprecated option was reverted, but we still need to make this breaking behavior change in 2.20. (cc @jason-ha as FYI)

cc @jzaffiro for docs review. Thanks!

@MarioJGMsoft / @markfields, since the API change was moved out to 2.30, can you backfill with a replacement breaking change issue under 2.20 list?

@anthony-murphy, I think you spotted the Word incompatibility. Are you able to confirm that this doesn't similarly impact Word (as the API change did)?

MarioJGMsoft · 2025-01-21T22:23:08Z

@dannimad -- Marking this as release-blocking. The removal of this deprecated option was reverted, but we still need to make this breaking behavior change in 2.20. (cc @jason-ha as FYI)
cc @jzaffiro for docs review. Thanks!

@MarioJGMsoft / @markfields, since the API change was moved out to 2.30, can you backfill with a replacement breaking change issue under 2.20 list?

@anthony-murphy, I think you spotted the Word incompatibility. Are you able to confirm that this doesn't similarly impact Word (as the API change did)?

I added an issue to the breaking change list, let me know what you think about it.

fix: added assert for compression ON grouping OFF

70c9da0

github-actions bot added base: main PRs targeted against main branch area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc labels Jan 17, 2025

MarioJGMsoft changed the title ~~fix: added assert for compression ON grouping OFF~~ Added assert for compression ON grouping OFF Jan 17, 2025

markfields added 2 commits January 18, 2025 00:56

Ensure compression is disabled everywhere grouping is

eb0ea1b

Some places tried to use undefined to disable. This does not work, it gets replaced with default values.

Fix some tests

a9497a0

markfields reviewed Jan 18, 2025

View reviewed changes

packages/runtime/container-runtime/src/containerRuntime.ts Outdated Show resolved Hide resolved

markfields added 2 commits January 18, 2025 22:57

More test fixes

3d523f7

Add changeset and update to UsageError.

50a5c6a

github-actions bot added the changeset-present label Jan 20, 2025

markfields marked this pull request as ready for review January 20, 2025 17:38

markfields requested a review from a team as a code owner January 20, 2025 17:38

markfields added the release-blocking Must be addressed before we cut and publish the next release label Jan 20, 2025

markfields reviewed Jan 20, 2025

View reviewed changes

packages/test/test-version-utils/src/compatUtils.ts Show resolved Hide resolved

markfields added 2 commits January 20, 2025 19:37

Stress Tests: Disable compression when grouping is disabled

14cfbcd

Merge branch 'main' into removeBatchGroupingChangeset

373684b

markfields changed the title ~~Added assert for compression ON grouping OFF~~ BREAKING IN 2.20] Added assert for compression ON grouping OFF Jan 20, 2025

markfields reviewed Jan 20, 2025

View reviewed changes

markfields approved these changes Jan 20, 2025

View reviewed changes

MarioJGMsoft changed the title ~~BREAKING IN 2.20] Added assert for compression ON grouping OFF~~ [BREAKING IN 2.20] Added assert for compression ON grouping OFF Jan 21, 2025

jzaffiro approved these changes Jan 21, 2025

View reviewed changes

markfields added 3 commits January 21, 2025 20:55

Change default behavior if compression options are not specified

d926047

Merge branch 'main' into removeBatchGroupingChangeset

402afde

Test fix

070b19d

markfields reviewed Jan 21, 2025

View reviewed changes

MarioJGMsoft linked an issue Jan 21, 2025 that may be closed by this pull request

Removal of op compression without op grouping in v2.20 #23618

Closed

MarioJGMsoft merged commit 92b695a into microsoft:main Jan 21, 2025
28 checks passed

github-actions bot mentioned this pull request Jan 21, 2025

Upcoming Release: FluidFramework v2.21.0 #22101

Open

MarioJGMsoft deleted the removeBatchGroupingChangeset branch January 22, 2025 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAKING IN 2.20] Added assert for compression ON grouping OFF #23608

[BREAKING IN 2.20] Added assert for compression ON grouping OFF #23608

MarioJGMsoft commented Jan 17, 2025 •

edited by markfields

Loading

markfields commented Jan 20, 2025 •

edited

Loading

markfields Jan 20, 2025

markfields Jan 20, 2025 •

edited

Loading

markfields Jan 20, 2025

jzaffiro left a comment

markfields Jan 21, 2025

jason-ha commented Jan 21, 2025

MarioJGMsoft commented Jan 21, 2025

[BREAKING IN 2.20] Added assert for compression ON grouping OFF #23608

[BREAKING IN 2.20] Added assert for compression ON grouping OFF #23608

Conversation

MarioJGMsoft commented Jan 17, 2025 • edited by markfields Loading

Description

Acceptance Criteria

Execution Plan

Reviewer Guidance

markfields commented Jan 20, 2025 • edited Loading

markfields Jan 20, 2025

Choose a reason for hiding this comment

markfields Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

markfields Jan 20, 2025

Choose a reason for hiding this comment

jzaffiro left a comment

Choose a reason for hiding this comment

markfields Jan 21, 2025

Choose a reason for hiding this comment

jason-ha commented Jan 21, 2025

MarioJGMsoft commented Jan 21, 2025

MarioJGMsoft commented Jan 17, 2025 •

edited by markfields

Loading

markfields commented Jan 20, 2025 •

edited

Loading

markfields Jan 20, 2025 •

edited

Loading