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.

Sign up for GitHub

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

Jump to bottom

Fix "savedOp" metadata property propagation for grouped ops #20837

Merged
merged 2 commits into from
Apr 25, 2024
Merged

Fix "savedOp" metadata property propagation for grouped ops #20837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba2a029
SavedOp for grouped ops
vladsud Apr 24, 2024
6ee29a0
PR feedback
vladsud Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions packages/runtime/container-runtime/src/containerRuntime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ import {
type OutboundContainerRuntimeMessage,
type UnknownContainerRuntimeMessage,
} from "./messageTypes.js";
import { IBatchMetadata, IIdAllocationMetadata } from "./metadata.js";
import { IBatchMetadata, ISavedOpMetadata } from "./metadata.js";
import {
BatchMessage,
IBatch,
Expand Down Expand Up @@ -660,11 +660,13 @@ type MessageWithContext =
message: InboundSequencedContainerRuntimeMessage;
modernRuntimeMessage: true;
local: boolean;
savedOp?: boolean;
}
| {
message: InboundSequencedContainerRuntimeMessageOrSystemMessage;
modernRuntimeMessage: false;
local: boolean;
savedOp?: boolean;
};

const summarizerRequestUrl = "_summarizer";
Expand Down Expand Up @@ -1126,7 +1128,7 @@ export class ContainerRuntime
// Id Compressor serializes final state (see getPendingLocalState()). As result, it needs to skip all ops that preceeded that state
// (such ops will be marked by Loader layer as savedOp === true)
// That said, in "delayed" mode it's possible that Id Compressor was never initialized before getPendingLocalState() is called.
// In such case we have to process all ops, including those marked with saveOp === true.
// In such case we have to process all ops, including those marked with savedOp === true.
private readonly skipSavedCompressorOps: boolean;

public get idCompressorMode() {
Expand Down Expand Up @@ -2557,6 +2559,7 @@ export class ContainerRuntime
// We do not need to make a deep copy. Each layer will just replace message.contents itself,
// but will not modify the contents object (likely it will replace it on the message).
const messageCopy = { ...messageArg };
const savedOp = (messageCopy.metadata as ISavedOpMetadata)?.savedOp;
for (const message of this.remoteMessageProcessor.process(messageCopy)) {
const msg: MessageWithContext = modernRuntimeMessage
? {
Expand All @@ -2574,6 +2577,7 @@ export class ContainerRuntime
local,
modernRuntimeMessage,
};
msg.savedOp = savedOp;

// ensure that we observe any re-entrancy, and if needed, rebase ops
this.ensureNoDataModelChanges(() => this.processCore(msg));
Expand Down Expand Up @@ -2664,13 +2668,7 @@ export class ContainerRuntime
// stashed ops flow. The compressor is stashed with these ops already processed.
// That said, in idCompressorMode === "delayed", we might not serialize ID compressor, and
// thus we need to process all the ops.
if (
!(
this.skipSavedCompressorOps &&
(messageWithContext.message.metadata as IIdAllocationMetadata)?.savedOp ===
true
)
) {
if (!(this.skipSavedCompressorOps && messageWithContext.savedOp === true)) {
const range = messageWithContext.message.contents;
// Some other client turned on the id compressor. If we have not turned it on,
// put it in a pending queue and delay finalization.
Expand Down
4 changes: 2 additions & 2 deletions packages/runtime/container-runtime/src/metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ export interface IBlobMetadata {
}

/**
* The IdCompressor needs to know if this is a replayed savedOp as those need to be skipped in stashed ops scenarios.
* ContainerRuntime needs to know if this is a replayed savedOp as those need to be skipped in stashed ops scenarios.
*/
export interface IIdAllocationMetadata {
export interface ISavedOpMetadata {
savedOp?: boolean;
}
Loading