Process ops by batches in remoteMessageProcessor and pendingStateManager #21785
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
reviewed
Jul 8, 2024
packages/runtime/container-runtime/src/opLifecycle/remoteMessageProcessor.ts
Outdated
Show resolved
Hide resolved
markfields
reviewed
Jul 8, 2024
packages/runtime/container-runtime/src/opLifecycle/remoteMessageProcessor.ts
Outdated
Show resolved
Hide resolved
markfields
reviewed
Jul 8, 2024
packages/runtime/container-runtime/src/opLifecycle/remoteMessageProcessor.ts
Show resolved
Hide resolved
markfields
reviewed
Jul 8, 2024
⯅ @fluid-example/bundle-size-tests: +2.39 KB
Baseline commit: 7edcd04 |
markfields
reviewed
Jul 8, 2024
markfields
reviewed
Jul 8, 2024
markfields
reviewed
Jul 8, 2024
markfields
reviewed
Jul 8, 2024
| @@ -67,13 +67,14 @@ export class RemoteMessageProcessor { | |||
| * 3. If grouped, ungroup the message | |||
| * For more details, see https://github.com/microsoft/FluidFramework/blob/main/packages/runtime/container-runtime/src/opLifecycle/README.md#inbound | |||
| * | |||
| * @returns the unchunked, decompressed, ungrouped, unpacked SequencedContainerRuntimeMessages encapsulated in the remote message. | |||
| * @returns all the unchunked, decompressed, ungrouped, unpacked InboundSequencedContainerRuntimeMessage from a single batch | |||
| * or undefined if the batch is not yet complete. | |||
| * For ops that weren't virtualized (e.g. System ops that the ContainerRuntime will ultimately ignore), | |||
| * a singleton array [remoteMessageCopy] is returned | |||
| */ | |||
| public process(remoteMessageCopy: ISequencedDocumentMessage): | |||
There was a problem hiding this comment.
May not matter, but I think this type could also be InboundSequencedContainerRuntimeMessage.
markfields
reviewed
Jul 8, 2024
packages/runtime/container-runtime/src/opLifecycle/remoteMessageProcessor.ts
Outdated
Show resolved
Hide resolved
markfields
reviewed
Jul 8, 2024
packages/runtime/container-runtime/src/opLifecycle/remoteMessageProcessor.ts
Outdated
Show resolved
Hide resolved
markfields
reviewed
Jul 8, 2024
markfields
reviewed
Jul 8, 2024
| message: InboundSequencedContainerRuntimeMessage; | ||
| localOpMetadata: unknown; | ||
| }[] { | ||
| return batch.map((message) => ({ |
There was a problem hiding this comment.
Future: we'll be able to move maybeProcessBatchStart and maybeProcessBatchEnd to the start/end of this function an drop the maybe :)
markfields
approved these changes
Jul 8, 2024
dannimad
added a commit
that referenced
this pull request
Jul 19, 2024
…ior (#21879) ## Description In #21785, we inadvertently removed the following two function calls from the incoming op processing code in the case of `!modernRuntimeMessage`: * `ensureContentsDeserialized` * `unpackRuntimeMessage` The second is obviously fine, we early-return on the same condition as `modernRuntimeMessage`. The first is _probably_ fine, but there's some ambiguity. It's possible that there are some legacy runtime messages or system messages with an unexpected format where we need to do this deserialization, and we must not break those cases. So this PR adds back that call (moving it to `ContainerRuntime.process`, and also instruments these two functions so we can collect logs to see if we hit certain codepaths here at scale. --------- Co-authored-by: Daniel Madrid <105010181+dannimad@users.noreply.github.com>
dannimad
added a commit
that referenced
this pull request
Sep 13, 2024
markfields
added a commit
that referenced
this pull request
Sep 16, 2024
…g for the whole batch (#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. ### How the change works 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: - 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. #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.
This was referenced Sep 16, 2024
frankmueller-msft
pushed a commit
that referenced
this pull request
Sep 16, 2024
…or and pendingStateManager (#22509) Reverting back #21785 given discoveries described here [AB#15212](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/15212)
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.
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.
In order to be able to recognize a batch that becomes empty on resubmit, we need to process them by batch instead of by individual op. This will help to make subsequent changes that will allow flushing on empty batches.