FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

Use fixed value for summarizer node's SummaryHandle id

Open agarwal-navin opened this issue 1 year ago • 2 comments

Background

This change is aimed at removing the tracking of basePath, localPath and additionalPath from SummarizerNode. These are used in case a node did not change and it's summary is a SummaryHandle. These paths are used to generate the SummaryHandle's id.

There is no need to track these three paths across summaries of this node because they don't change anymore. They make the summarizer node code complicated and hard to understand / make changes. Previously, these paths could change from one summary to another because of 2 reasons:

  1. When transitioning to writing summary trees under .channels sub-tree. We have to support loading from snapshot where summaries were not written under .channels sub-tree. This PR adds a fix for that - Regenerate the first summary (i.e., don't use incremental summaries) when loading from such snapshots.
  2. Differential summaries where if a node failed to summarize, we would use the previous summary + a blob where its ops would be stored. This is totally removed now and is not supported.

Description

This PR is one of 2 ways we can acheive removing these paths. The second one is https://github.com/microsoft/FluidFramework/pull/19014.

Note: It does not remove basePath, localPath and additionalPath from SummarizerNode yet. It removes its usage and can be removed. I didn't remove it to make the change smaller and easier to review.

In this PR, instead of tracking the above paths, the summarizer node is given the handle id to be used for SummaryHandle. This does not change thoughout the life time of the node. When creating a child node, its id and a prefix to be used for handle id is given as a parameter to the parent. The parent node uses this information to build the handle id for the child and passes it during creation.

AB#3740

agarwal-navin avatar Dec 30 '23 01:12 agarwal-navin

@fluid-example/bundle-size-tests: +1.45 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 741.54 KB 741.79 KB +250 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 540.51 KB 540.74 KB +228 Bytes
loader.js 166.8 KB 166.82 KB +22 Bytes
map.js 559.06 KB 559.29 KB +239 Bytes
matrix.js 657.29 KB 657.52 KB +239 Bytes
odspDriver.js 90.24 KB 90.26 KB +22 Bytes
odspPrefetchSnapshot.js 41.82 KB 41.83 KB +11 Bytes
sharedString.js 676.01 KB 676.25 KB +239 Bytes
sharedTree.js 790.65 KB 790.87 KB +228 Bytes
Total Size 4.28 MB 4.28 MB +1.45 KB

Baseline commit: db64250d680465f09f4b0872a5e89417c3538d94

Generated by :no_entry_sign: dangerJS against dfcc1e6c105e528937b9b34e22eb955b592f8c9c

msfluid-bot avatar Dec 30 '23 01:12 msfluid-bot

Also, are you confident in existing test coverage? The actual change here is small, but fundamental (switching from fullPath to the passed in handle).

markfields avatar Jan 02 '24 23:01 markfields