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

disable message batching for Azure customers #11568

Merged
merged 4 commits into from
Aug 19, 2022

Conversation

alpaix
Copy link
Contributor

@alpaix alpaix commented Aug 17, 2022

The problem

The problem reported by a customer via IcM. Attempting to sync a large data set consisting of batched multiple messages (less than 768kb each) will hit Socket.io buffer max size and the connection will close. The reason is the delta manager flushes batched messages periodically without enforcing the max op size. The max op size is only honored by the container runtime per each message submitted as imposed by the initial design.

The solution

Disable the message batching by setting flushMode to Intermediate for Azure customers. The workaround should be reconsidered when the original batching problem is resolved.

@alpaix alpaix requested review from skylerjokiel and znewton August 17, 2022 06:38
@github-actions github-actions bot added area: loader Loader related issues 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 Aug 17, 2022
@alpaix alpaix marked this pull request as ready for review August 17, 2022 17:41
@alpaix alpaix requested review from a team as code owners August 17, 2022 17:41
Copy link
Contributor

@skylerjokiel skylerjokiel left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's get a few more eyes on this before it goes in.

Copy link
Contributor

@znewton znewton left a comment

Choose a reason for hiding this comment

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

I like it. Maybe get @vladsud's and/or @andre4i's eyes?

Copy link
Contributor

@andre4i andre4i left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is the direction we want to go here. We have work tracking for this problem at https://dev.azure.com/fluidframework/internal/_workitems/edit/465

@skylerjokiel skylerjokiel self-requested a review August 17, 2022 19:10
@github-actions github-actions bot removed public api change Changes to a public API area: runtime Runtime related issues area: loader Loader related issues labels Aug 18, 2022
@alpaix
Copy link
Contributor Author

alpaix commented Aug 18, 2022

A better solution would be explicitly setting the value of flushMode to Immediate. Will explore different options where and how to change it. Meanwhile, in this PR iteration I added an end-to-end test showcasing a workaround for the batch size limit problem. The test infrastructure allows setting custom runtime options that is used to override the flush mode value.

@alpaix alpaix requested a review from a team as a code owner August 18, 2022 17:13
@github-actions github-actions bot added the area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct label Aug 18, 2022
@alpaix
Copy link
Contributor Author

alpaix commented Aug 18, 2022

Scoped down the fix to a single line change in fluid-static. Additionally, added an end-to-end test to verify Immediate mode is capable of handling a sequence of large messages.
@andre4i @znewton @ChumpChief

@alpaix alpaix changed the title enforce max batch size when submitting batched container messages disable message batching for Azure customers Aug 18, 2022
@alpaix alpaix merged commit 4750002 into microsoft:main Aug 19, 2022
@alpaix alpaix deleted the op-size-limit branch August 19, 2022 01:58
@github-actions
Copy link
Contributor

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

alpaix added a commit to alpaix/FluidFramework that referenced this pull request Aug 24, 2022
* Add e2e test to demo use of flush mode

* override flush mode for azure clients

* disable new test for local driver

* PR feedback
skylerjokiel pushed a commit that referenced this pull request Aug 24, 2022
alpaix added a commit to alpaix/FluidFramework that referenced this pull request Aug 25, 2022
* Add e2e test to demo use of flush mode

* override flush mode for azure clients

* disable new test for local driver

* PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants