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

summarizeAsync #8592

Merged
merged 11 commits into from
Jan 6, 2022
Merged

summarizeAsync #8592

merged 11 commits into from
Jan 6, 2022

Conversation

scarlettjlee
Copy link
Contributor

@scarlettjlee scarlettjlee commented Dec 20, 2021

This PR is for parts 1 and 2 of Vlad's comment here on prior abandoned PR.

Add a new method on IChannel (initially optional for compat) to do summarization asynchronously and only call this during regular summarization, not attach.

SharedObject is split into SharedObjectCore and SharedObject, where Core does not have implementations for summarize or GC. DDS implementers are free to implement as they like. For existing DDSs in this repo, summarizeAsync will call into summarizeCore (previously snapshotCore) as synchronous summarize does.

@scarlettjlee scarlettjlee added this to the December 2021 milestone Dec 20, 2021
@scarlettjlee scarlettjlee self-assigned this Dec 20, 2021
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: runtime Runtime related issues public api change Changes to a public API labels Dec 20, 2021
@github-actions github-actions bot requested a review from curtisman December 20, 2021 12:18
extends EventEmitterWithErrorHandling<TEvent> implements ISharedObject<TEvent> {
/**
* @param obj - The thing to check if it is a SharedObject
* @returns Returns true if the thing is a SharedObject
*/
public static is(obj: any): obj is SharedObject {
public static is(obj: any): obj is SharedObjectCore {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not directly related to your changes, but I believe this is wrong - opened #8594 to track it

Copy link
Contributor

@vladsud vladsud left a comment

Choose a reason for hiding this comment

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

Looks great! Just minor comments on naming

@scarlettjlee scarlettjlee linked an issue Jan 3, 2022 that may be closed by this pull request
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jan 3, 2022

@fluid-example/bundle-size-tests: -1.79 KB
Metric NameBaseline SizeCompare SizeSize Diff
container.js 176.59 KB 176.43 KB -161 Bytes
map.js 50.65 KB 50.24 KB -414 Bytes
matrix.js 146.17 KB 145.64 KB -547 Bytes
odspDriver.js 189.3 KB 189.14 KB -159 Bytes
odspPrefetchSnapshot.js 44.28 KB 44.28 KB No change
sharedString.js 166.98 KB 166.45 KB -548 Bytes
Total Size 806.66 KB 804.87 KB -1.79 KB

Baseline commit: f60b531

Generated by 🚫 dangerJS against e3050b8

@github-actions github-actions bot added the breaking change This PR or issue would introduce a breaking change label Jan 3, 2022
@scarlettjlee scarlettjlee marked this pull request as ready for review January 3, 2022 22:25
@scarlettjlee scarlettjlee requested a review from a team as a code owner January 3, 2022 22:25
const blobContent = serializer.stringify(content, this.handle);
const builder = new SummaryTreeBuilder();
builder.addBlob(snapshotFileName, blobContent);
return builder.getSummaryTree();
Copy link
Contributor

Choose a reason for hiding this comment

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

The cleanup to use SummaryTreeBuilder() everywhere is very nice, thank you.

@github-actions github-actions bot requested review from vladsud and DLehenbauer and removed request for a team January 5, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: propertydds area: dds: sharedstring area: dds Issues related to distributed data structures area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc breaking change This PR or issue would introduce a breaking change public api change Changes to a public API
Projects
None yet
4 participants