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

Summary docs #10435

Merged
merged 17 commits into from
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
6 changes: 2 additions & 4 deletions api-report/runtime-definitions.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ export interface ISummarizerNodeWithGC extends ISummarizerNode {
updateUsedRoutes(usedRoutes: string[], gcTimestamp?: number): void;
}

// @public (undocumented)
// @public
export interface ISummaryStats {
// (undocumented)
blobNodeCount: number;
Expand All @@ -368,11 +368,9 @@ export interface ISummaryStats {
unreferencedBlobSize: number;
}

// @public (undocumented)
// @public
export interface ISummaryTreeWithStats {
// (undocumented)
stats: ISummaryStats;
// (undocumented)
summary: ISummaryTree;
}

Expand Down
60 changes: 54 additions & 6 deletions common/lib/protocol-definitions/src/summary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,41 +21,89 @@ export interface ISummaryCommitter {
date: string;
}

/**
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

Orthogonal to this PR - Interesting to note that ISummaryAuthor and ISummaryCommiter above are never used. We may want to look into removing them. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking by #10456

* Represents a leaf node from the Summary Tree.
andre4i marked this conversation as resolved.
Show resolved Hide resolved
*/
// eslint-disable-next-line @typescript-eslint/no-namespace
export namespace SummaryType {
export type Tree = 1;
export type Blob = 2;
export type Handle = 3;
export type Attachment = 4;

export const Tree: Tree = 1 as const;
export const Blob: Blob = 2 as const;
export const Handle: Handle = 3 as const;
export const Attachment: Attachment = 4 as const;
/**
* Another recursive data structure.
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

This should probably say that this represents a sub-tree in the summary. #Resolved

*/
export const Tree: Tree = 1 as const;

/**
* Binary data to be uploaded to the server. The data is sent to the drivers
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

I don't think we should mention driver or server here. Runtime doesn't really care about any of that.

This should probably just say that this represents a blob of data that is added to the summary. Such as the user data that is added to the DDS or metadata added by runtime such as data store / channel attributes. #Resolved

* and each driver will decide how the blob will be uploaded to the server.
*/
export const Blob: Blob = 2 as const;
/**
andre4i marked this conversation as resolved.
Show resolved Hide resolved
* Path to an already stored tree that hasn't changed since the last summary.
*/
export const Handle: Handle = 3 as const;

/**
* Unique identifier to larger blobs uploaded outside of the summary.
* Ex. DDS has large images or video that will be uploaded by the BlobManager and
* receive an Id that can be used in the summary.
*/
export const Attachment: Attachment = 4 as const;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If you happen to know the answer, it would be good to document if there was an explicit choice not to declare SummaryType as an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anthony-murphy Do you know why ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe because of the reason mentioned in this comment here - #9548 (comment)

export type SummaryType = SummaryType.Attachment | SummaryType.Blob | SummaryType.Handle | SummaryType.Tree;

export type SummaryTypeNoHandle = SummaryType.Tree | SummaryType.Blob | SummaryType.Attachment;

/**
* Path to a summary tree object from the last uploaded summary indicating the summary object hasn't
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

Suggested change
* Path to a summary tree object from the last uploaded summary indicating the summary object hasn't
* Path to a summary tree object from the last successful summary indicating the summary object hasn't
``` #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't very important. I am just trying to differentiate the case where the last uploaded summary was not successfully ack'd by the server.

* changed since it was uploaded.
* To illustrate, if a DataStore did not get any ops since last summary, the framework runtime will use a handle for the
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

This should say if a Datastore did not change since last summary. The fact that we use ops is an implementation detail. There could be other factors leading to a data store being summarized - such as its GC state changing from last summary.
Similar comment for DDS. #Resolved

* entire DataStore instead of re-sending the entire subtree. Same concept will be applied for a DDS.
* If a DDS did not receive any ops since the last summary, the ISummary tree for that DDS will not have changed and
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

From a readability point of view, I don't think this line about DDS is needed. Mentioning that the behavior is same as data store should be sufficient. #Resolved

* the fluid framework will send that DDS' handle so the server can use that previous handle instead of sending the
* entire DDS. An example of handle would be: '/<DataStoreId>/<DDSId>'.
* Notice that handles are optimizations from the Fluid Framework Runtime and the DDS is not aware of the handles.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see two issues with this:
Shouldn't that be "SharedObjects" and not DDS.

Also SharedObject authors have to implement a method that returns a datastructure that can use this type.
Is this comment intendting to say that a SharedObject subclass should never use ISummaryHandle? Is so, we should adjust the types so they can't return them as part of the summary tries they produce.

* Also, the use of Summary Handles is currently restricted to DataStores and DDS.
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

You should omit this last line. We are working on adding handles for GC data and we will soon add it in other places too. #Resolved

*/
export interface ISummaryHandle {
type: SummaryType.Handle;

// No handles, all other SummaryType are Ok
/**
* Type of Summary Handle all Summary types with the exception of handles (which are NOT supported).
andre4i marked this conversation as resolved.
Show resolved Hide resolved
*/
handleType: SummaryTypeNoHandle;

// Stored handle reference
/**
* Unique path that identifies the stored handle reference.
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

Suggested change
* Unique path that identifies the stored handle reference.
* Unique path that identifies the corresponding sub-tree in a previous summary.
``` #Resolved

*/
handle: string;
Copy link
Contributor

@CraigMacomber CraigMacomber May 26, 2022

Choose a reason for hiding this comment

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

How do I get one of these?

Copy link
Contributor Author

@NicholasCouri NicholasCouri May 27, 2022

Choose a reason for hiding this comment

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

As a developer, I don't think you need to get it - if I got it right, it is automatically set (id) when we create the fluid file from a summary: Ex. createNewFluidFileFromSummary.

@agarwal-navin to confirm.

Copy link
Contributor

@agarwal-navin agarwal-navin May 27, 2022

Choose a reason for hiding this comment

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

Not really. Currently, every object that wants to use this to send a path to previous summary instead of full summary has to know what the path is. For example, the summarizer node for data store context and channel contexts tracks this path and adds it.
This path is basically its path in the container's object tree. For instance, for DDS it is /<dataStoreId>/<ddsId>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the way it should it be is similar to how summary tree is built. Each node just adds its summary tree when asked for it. It's the responsibility of the caller to add this tree against the correct path. This way by the time the summary tree reaches the root, all paths are fully built w.r.t. root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's sync up ?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as a reference- the only place I found that adds the handle to the tree is when we call encodeSummary (builder.addHandle).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the main usage of summary handle in our code -

.

The encodeSummary bit is part of this feature called differential summary which we don't use anymore.

Copy link
Contributor Author

@NicholasCouri NicholasCouri May 27, 2022

Choose a reason for hiding this comment

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

Thanks Navin I had missed it - at the SummarizerNode level, if we are tracking it, it is stored under _latestSummary.fullPath.path

}

/**
Copy link
Contributor

@CraigMacomber CraigMacomber May 26, 2022

Choose a reason for hiding this comment

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

I think it would be good to mention here that to reference an already uploaded blob use ISummaryAttachment. Someone looking for how to do that is likley to find themselves here, and not realize they need to look into ISummaryAttachments. #Resolved

* String or Binary data to be uploaded to the server as part of the document's Summary.
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

nit: container's summary. We try to avoid using the term document as it is ambiguous. #Resolved

*/
export interface ISummaryBlob {
type: SummaryType.Blob;
content: string | Uint8Array;
}

/**
* Handle to blobs uploaded outside of the summary. Attachment Blobs are uploaded and downloaded separately via
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

We should avoid using the term handle here as it adds confusion with the other handle. This should probably say something to the effect of id uniquely represents the attachment blobs uploaded directly to storage. #Resolved

* http requests and are not included on the snapshot payload. The ISummaryAttachment are handles to these blobs.
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

I don't think this should say http requests as it is not just restricted to http. #Resolved

Copy link
Contributor

@kian-thompson kian-thompson May 26, 2022

Choose a reason for hiding this comment

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

Suggested change
* http requests and are not included on the snapshot payload. The ISummaryAttachment are handles to these blobs.
* http requests and are not included on the snapshot payload. The ISummaryAttachment are handles to these blobs.

nit
Could also be interesting to include an example of an attachment blob like above. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added - let me know if it helps.

* Additional information can be found here: https://github.com/microsoft/FluidFramework/issues/6374
Copy link
Contributor

@CraigMacomber CraigMacomber May 26, 2022

Choose a reason for hiding this comment

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

This documentation on ISummaryAttachment is super useful to me even in its current state! #Resolved

*/
export interface ISummaryAttachment {
type: SummaryType.Attachment;
id: string;
}
Comment on lines 104 to 105
Copy link
Contributor

@CraigMacomber CraigMacomber May 26, 2022

Choose a reason for hiding this comment

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

What format is this in OR how do I get one from a IFluidHandle? Is it the absolute path? A json seralized handle? Something else? #Resolved

Copy link
Contributor Author

@NicholasCouri NicholasCouri May 27, 2022

Choose a reason for hiding this comment

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

Added an example, it is an id returned from the backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used so I am not really sure. I believe the idea is that id here should be the id that storage gives for this attachment. @vladsud do you know the usage pattern for this?

Copy link
Contributor Author

@NicholasCouri NicholasCouri May 27, 2022

Choose a reason for hiding this comment

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

I added an example that makes it easier to visualize. @navin, are you sure this is not used ? After all, we do write it :)

Ex. ".blobs": {
"type": 1,
"tree": {
"0": {
"id": "bQAQKARDdMdTgqICmBa_ZB86YXwGP",
"type": 4
}
}
},


/**
* Tree Node data structure with children that are nodes of SummaryObject type:
* Blob, Handle, Attachment or another Tree.
*/
export interface ISummaryTree {
type: SummaryType.Tree;

Expand Down
6 changes: 6 additions & 0 deletions packages/runtime/datastore-definitions/src/channel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,19 @@ export interface IChannel extends IFluidLoadable {

/**
* Generates summary of the channel synchronously.
* @param fullTree - flag indicating whether the attempt should generate a full
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

It may be worth adding that this is called when an attach message for a local channel is generated. In other words, when the channel is being attached to make it visible to other clients. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

How important is this "attempt"?

Is it a bug or just a perf issue if the implementation returns a summary tree contining a handle for an unchanged subtree? What about a handle for something other than an unchaed subtree (es: a new blob)?

Its important for the documenration on this method to make it possible to tell if an imlementation of it is incorrect, and without answering the above, there are cases where I wouldn't know.

Copy link
Contributor

Choose a reason for hiding this comment

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

The attach summary cannot contain summary handles, i.e., path to sub-trees (or blobs) in the previous summary. It can however contain ISummaryAttachment, i.e., handles to blobs that were uploaded async via the blob manager.
Since attach summary is generated for local channels when making them visible to remote clients, they don't have any previous summaries to compare against. So, they cannot really send summary handles.

I agree with you that its worth calling it out here.

* summary tree without any handles for unchanged subtrees.
* @param trackState - This tells whether we should track state from this summary.
Copy link
Contributor

@CraigMacomber CraigMacomber May 26, 2022

Choose a reason for hiding this comment

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

If I'm implementing this interface, how do I "trackState"? What does that mean? How can I tell if code does it correctly or not? Thats what this comment needs to answer.

Also from a verbosity standpoint, "This tells whether we should" does not really provide any information, and "we" isn't clear (this is a contract between caller and calee: which is "we"?). I'd prefer something like:

"If true, the implementation must ?????" #Resolved

Copy link
Contributor Author

@NicholasCouri NicholasCouri May 27, 2022

Choose a reason for hiding this comment

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

I believe that for Summaries ( the same applies for the GC summarization?) , at least when we do the summary of the runtime at a specific sequence number, the default behavior is to always track the state of every object.

@agarwal-navin to make sure it is correct. Can you confirm which scenarios we do not track the state and why would we not track it ?

I know if it is false we bypass the Summarizer node call and go straight to summarizeInternal.

Copy link
Contributor

Choose a reason for hiding this comment

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

State tracking is basically an optimization that we added to track state of objects across summaries. Basically, if the state of an object did not change since last successful summary, we can send an ISummaryHandle for this object instead of re-summarizing it.
If this is false, the expectation is that you should never send an ISummaryHandle since you are not expected to track state.

This was mostly internal and the state tracking happens in summarizer node for data store context and channel context. If a DDS wants to track its internal summary state, it is free to do so (but no DDS does this).

Copy link
Contributor

Choose a reason for hiding this comment

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

There are plans to do this at DDS level - https://dev.azure.com/fluidframework/internal/_workitems/edit/423. When this happens, this option should either be removed or it will make more sense because we would likely expose the previous successful summary. And then we say something like, if trackState is true, feel free to use this previous summary to send summary handles.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the requirement here is (and thus a good doc comment would be) - if false, the implementation must not return a tree containing any ISummaryHandles?.

Might be better to rename this "allowHandles"?

It would be nice to deduplicate this documentation (as well as fullTree) across the various places they occur. This could be done as a non-breaking change by adding a type alias (ex: export type TrackState = boolean;) and documenting that, or as a breaking change by switching to an enum, or a summary options object with trackState and fullTree fields document on it.

Copy link
Contributor Author

@NicholasCouri NicholasCouri May 31, 2022

Choose a reason for hiding this comment

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

I will leave this change to a different PR.

* @returns A tree representing the summary of the channel.
*/
getAttachSummary(fullTree?: boolean, trackState?: boolean): ISummaryTreeWithStats;

/**
* Generates summary of the channel asynchronously.
* This should not be called where the channel can be modified while summarization is in progress.
* @param fullTree - flag indicating whether the attempt should generate a full
Copy link
Contributor

@agarwal-navin agarwal-navin May 26, 2022

Choose a reason for hiding this comment

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

It may be worth adding that this is called when generating a summary of the entire container. #Resolved

* summary tree without any handles for unchanged subtrees.
* @param trackState - This tells whether we should track state from this summary.
* @returns A tree representing the summary of the channel.
Copy link
Contributor

@CraigMacomber CraigMacomber May 26, 2022

Choose a reason for hiding this comment

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

I know you didn't change this, but I think it should be improved.

We can remove some verbosity, and be more specific (ideally something more specific that what I said would be good, but I don't know better). From context we already know this returns the summary of the channel (its channel.summarize afterall), so details beyond that are useful.

Suggested change
* @returns A tree representing the summary of the channel.
* @returns A summary capturing the current state of the channel.
``` #Resolved

*/
summarize(fullTree?: boolean, trackState?: boolean): Promise<ISummaryTreeWithStats>;
Expand Down
14 changes: 14 additions & 0 deletions packages/runtime/runtime-definitions/src/summary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import {
IGarbageCollectionSummaryDetails,
} from "./garbageCollection";

/**
* Contains the aggregation data from a Tree/Subtree.
*/
export interface ISummaryStats {
treeNodeCount: number;
blobNodeCount: number;
Expand All @@ -24,8 +27,19 @@ export interface ISummaryStats {
unreferencedBlobSize: number;
}

/**
* The summarization methods are recursively invoked during the summary calculation and will
* compose the Summary Tree. At the same time, each component that is taking part of the summarization
* will populate the tree information aggregation for its subtree through the ISummaryStats interface.
* Any component that implements IChannelContext, IFluidDataStoreChannel or extends SharedObject
* will be taking part of the summarization process.
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, this description may not be very relevant here. This is essentially describing the summarization process. It should rather say what this interface means. Basically, that this represents the summary tree for a node along with the statistics for that tree. As an example it could mention that for a data store this has the data for the data store along with a subtree for each of its DDS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another important piece of information here may be its distinction from ISummarizeResult below.
Take a look at this issue which may help with some of the comments here - #4434

Copy link
Contributor

Choose a reason for hiding this comment

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

Its also worth adding a comment for ISummarizeInternalResult as its not clear what it is and how it differs from ISummarizeResult

*/
export interface ISummaryTreeWithStats {
/** Represents an agreggation of node counts and blob sizes associated to the current summary information */
stats: ISummaryStats;
/** Summary defines a recursive data structure that will be will be converted to a snapshot tree and uploaded
andre4i marked this conversation as resolved.
Show resolved Hide resolved
* to the backend.
*/
summary: ISummaryTree;
}

Expand Down