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

Prototype putting blobs in summary #21701

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Prototype putting blobs in summary #21701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,11 +361,15 @@
"args": [
// "--fgrep", // Uncomment to filter by test case name
// "<test case name>",
"--no-timeouts",
// "--no-timeouts",
"--exit",
],
"cwd": "${fileDirname}",
"skipFiles": ["<node_internals>/**", "**/node_modules/**"],
"skipFiles": [
"<node_internals>/**",
"**/node_modules/**",
"!**/node_modules/**/@fluidframework*/**",
],
"outFiles": [
// This config avoids loading dependent packages' test files, while using the same technique
// as the "Debug Current Mocha Test (*)" config to load source maps for test files in the current
Expand Down
7 changes: 7 additions & 0 deletions packages/drivers/routerlicious-driver/src/summaryTreeUploadManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,13 @@ export class SummaryTreeUploadManager implements ISummaryUploadManager {
);
return tryId;
}
case SummaryType.Attachment: {
assert(
path.length === 1,
"there should only be a single path part for attachment blobs",
);
return key;
}
default:
throw Error(`Unexpected handle summary object type: "${handleType}".`);
}
Expand Down
3 changes: 3 additions & 0 deletions packages/loader/container-loader/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,9 @@ function convertSummaryToSnapshotAndBlobs(summary: ISummaryTree): SnapshotWithBl
break;
}
case SummaryType.Handle: {
if (summaryObject.handleType === SummaryType.Attachment) {
break;
}
throw new LoggingError(
"No handles should be there in summary in detached container!!",
);
Expand Down
16 changes: 16 additions & 0 deletions packages/runtime/container-runtime/src/blobManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
ICreateBlobResponse,
ISnapshotTree,
ISequencedDocumentMessage,
SummaryType,
} from "@fluidframework/driver-definitions/internal";
import { canRetryOnError, runWithRetry } from "@fluidframework/driver-utils/internal";
import {
Expand All @@ -48,6 +49,7 @@ import {
import { v4 as uuid } from "uuid";

import { IBlobMetadata } from "./metadata.js";
import { blobsTreeName } from "./summary/index.js";

/**
* This class represents blob (long string)
Expand Down Expand Up @@ -728,6 +730,20 @@ export class BlobManager extends TypedEventEmitter<IBlobManagerEvents> {
),
);
}
if (this.redirectTable.size > 0) {
const redirectTree = new SummaryTreeBuilder(blobsTreeName);
Copy link
Contributor Author

@anthony-murphy anthony-murphy Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is the core of the change, just trying to move the redirect table from an opaque blob (above) to a summary tree. this keeps the blobs as attachment blobs. A change like this required changes to all layers, so very costly to rollout.

You can imagine also adding the ability to specify regular blob entries in this tree which have content, which could replace the detached case, but we'd need to keep attachment blobs for the attached case, since we upload them independently of summaries.

We could change blob upload to be a summary itself, that just incrementally adds a single blob, but that would also introduce concurrency issues between clients uploading blobs, and the summarizer due to the parent summary handle changing for every upload. to solve this, we'd likely need to send an op, which should work, and could be better than the current blob op, in that the server wouldn't need to read it, as every blob is already summarized at upload. However, there are problem where summarizer can never succeed if blobs keep getting uploaded, this is harder to solve without trade offs.

let blobLinked = false;
for (const [localId, storageId] of this.redirectTable) {
if (storageId !== undefined) {
blobLinked = true;
redirectTree.addHandle(localId, SummaryType.Attachment, storageId);
}
}
if (blobLinked) {
const blobTree = redirectTree.getSummaryTree();
builder.addWithStats(`${BlobManager.redirectTableBlobName}2`, blobTree);
}
}

return builder.getSummaryTree();
}
Expand Down
2 changes: 1 addition & 1 deletion packages/runtime/runtime-utils/api-report/runtime-utils.legacy.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export abstract class RuntimeFactoryHelper<T = IContainerRuntime> implements IRu

// @alpha (undocumented)
export class SummaryTreeBuilder implements ISummaryTreeWithStats {
constructor();
constructor(groupId?: string | undefined);
// (undocumented)
addAttachment(id: string): void;
// (undocumented)
Expand Down
3 changes: 2 additions & 1 deletion packages/runtime/runtime-utils/src/summaryUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,15 @@ export class SummaryTreeBuilder implements ISummaryTreeWithStats {
return {
type: SummaryType.Tree,
tree: { ...this.summaryTree },
groupId: this.groupId,
};
}

public get stats(): Readonly<ISummaryStats> {
return { ...this.summaryStats };
}

constructor() {
constructor(private readonly groupId?: string) {
this.summaryStats = mergeStats();
this.summaryStats.treeNodeCount++;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/test/test-end-to-end-tests/src/test/blobs.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ const ContainerCloseUsageError: ExpectedEvents = {
tinylicious: containerCloseAndDisposeUsageErrors,
};

describeCompat("blobs", "FullCompat", (getTestObjectProvider, apis) => {
describeCompat("blobs", "NoCompat", (getTestObjectProvider, apis) => {
const { SharedString } = apis.dds;
const testContainerConfig = makeTestContainerConfig([
["sharedString", SharedString.getFactory()],
Expand Down
5 changes: 4 additions & 1 deletion server/routerlicious/packages/test-utils/src/testDocumentStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ async function writeSummaryTreeObject(
return writeSummaryBlob(object.content, blobsShaCache, manager);
}
case SummaryType.Handle: {
if (snapshot === undefined) {
if (snapshot === undefined && object.handleType !== SummaryType.Attachment) {
throw Error("Parent summary does not exist to reference by handle.");
}
return getIdFromPath(object.handleType, object.handle, snapshot);
Expand Down Expand Up @@ -348,6 +348,9 @@ function getIdFromPathCore(
case SummaryType.Tree: {
return snapshot.trees[key]?.id;
}
case SummaryType.Attachment:{
return key;
}
default:
throw Error(`Unexpected handle summary object type: "${handleType}".`);
}
Expand Down
Loading