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

Prototype putting blobs in summary #21701

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

Conversation

anthony-murphy
Copy link
Contributor

in progress

@github-actions github-actions bot added area: contributor experience area: driver Driver related issues area: loader Loader related issues area: runtime Runtime related issues area: server Server related issues (routerlicious) 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 Jun 28, 2024
@@ -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

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.

Copy link
Contributor

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

Copy link
Contributor

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: contributor experience area: driver Driver related issues area: loader Loader related issues area: runtime Runtime related issues area: server Server related issues (routerlicious) area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant