ContainerRuntime: Process incoming batches op-by-op instead of waiting for the whole batch #22508
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area: runtime
markfields
force-pushed
the
o3/op-by-op-go
branch
from
753648f to
97cf37b
Compare
⯅ @fluid-example/bundle-size-tests: +1.32 KB
Baseline commit: da0c9fa |
dannimad
approved these changes
Sep 16, 2024
This was referenced Sep 16, 2024
markfields
added a commit
to markfields/FluidFramework
that referenced
this pull request
Sep 26, 2024
ContainerRuntime: Process incoming batches op-by-op instead of waiting for the whole batch (microsoft#22508) We are concerned that holding batch messages in ContainerRuntime even while DeltaManager advances its tracked sequence numbers through the batch could have unintended consequences. So this PR restores the old behavior of processing each message in a batch one-by-one, rather than holding until the whole batch arrives. Note that there's no change in behavior here for Grouped Batches. PR microsoft#21785 switched the RemoteMessageProcessor from returning ungrouped batch ops as they arrived, to holding them and finally returning the whole batch once the last arrived. The downstream code was also updated to take whole batches, whereas before it would take individual messages and use the batch metadata to detect batch start/end. Too many other changes were made after that PR to straight revert it. Logic was added throughout CR and PSM that looks at info about that batch which is found on the first message in the batch. So we can reverse the change and process one-at-a-time, but we need a way to carry around that "batch start info" with the first message in the batch. So we are now modeling the result that RMP yields as one of three cases: - A full batch of messages (could be from a single-message batch or a Grouped Batch) - The first message of a multi-message batch - The next message in a multi-message batch The first two cases include the "batch start info" needed for the recent Offline work. The third case just indicates whether it's the last message or not. "batch start info" and updating the downstream code to use that instead of reading it off the old "Inbound Batch" type. This PR now adds those other two cases to the RMP return type and handles processing them throughout CR and PSM.
frankmueller-msft
pushed a commit
that referenced
this pull request
Oct 1, 2024
…tches op-by-op instead of waiting for the whole batch) (#22654) ## Description Porting the following commits from `main` to `release/client/2.2`: * ce0a14c (Offline: Model empty batches differently, exposing the empty Grouped Batch message to more of the system) * e024b86 (ContainerRuntime: Refactor batch processing code to support either op-by-op or batch-all-at-once semantics) * 709f085 (ContainerRuntime: Process incoming batches op-by-op instead of waiting for the whole batch) ## Reviewer Guidance There was a ton of churn between 2.2 and 2.3 in this code (subset of [these changes to container-runtime dir](https://github.com/microsoft/FluidFramework/commits/release/client/2.3/packages/runtime/container-runtime?since=2024-08-17)), so the cherry-pick turned into more like a manual re-implementing of the change. In some ways it's a smaller diff compared to what needed to happen in `main`. So please review carefully as you would an original PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes AB#15221
Why
We are concerned that holding batch messages in ContainerRuntime even while DeltaManager advances its tracked sequence numbers through the batch could have unintended consequences. So this PR restores the old behavior of processing each message in a batch one-by-one, rather than holding until the whole batch arrives.
Note that there's no change in behavior here for Grouped Batches.
How
PR #21785 switched the RemoteMessageProcessor from returning ungrouped batch ops as they arrived, to holding them and finally returning the whole batch once the last arrived. The downstream code was also updated to take whole batches, whereas before it would take individual messages and use the batch metadata to detect batch start/end.
Too many other changes were made after that PR to straight revert it. Logic was added throughout CR and PSM that looks at info about that batch which is found on the first message in the batch. So we can reverse the change and process one-at-a-time, but we need a way to carry around that "batch start info" with the first message in the batch.
So we are now modeling the result that RMP yields as one of three cases:
The first two cases include the "batch start info" needed for the recent Offline work. The third case just indicates whether it's the last message or not.
#22501 added some of the necessary structure, introducing the type for "batch start info" and updating the downstream code to use that instead of reading it off the old "Inbound Batch" type. This PR now adds those other two cases to the RMP return type and handles processing them throughout CR and PSM.
Reviewer Guidance
Planning to port back to 2.2