Put channels in .channels subtrees in summary #5280
Conversation
arinwt
force-pushed
the
summary-name-collisions-write
branch
from
7eb03eb
to
d0f932a
Compare
|
Side note...
These two terms cause me a lot of confusion - is there a README or something that defines/contrasts them? Or is one just defunct and should be removed? |
The short answer is that "snapshot" is defunct and should be removed. Summary is the term we use for the serialized state at a given sequence number, commonly referred to as a "snapshot" or point in time. The term "summary" comes from the idea that it is a summary of the op log, it just consolidates it into a single structure. There is sort of an unintentional(?) distinction in the code: the snapshot types are used to represent the downloaded format, and the summary types for the uploaded format. So we upload an |
arinwt
force-pushed
the
summary-name-collisions-write
branch
from
e3827a8
to
791c9ab
Compare
| export type WriteFluidDataStoreAttributes = IFluidDataStoreAttributes2; | ||
|
|
||
| export function getAttributesFormatVersion(attributes: ReadFluidDataStoreAttributes): number { | ||
| if ("summaryFormatVersion" in attributes) { |
There was a problem hiding this comment.
Is if (attributes.summaryFormatVersion) a more definitive check here?
There was a problem hiding this comment.
Do you mean in case it's undefined? Typescript won't allow that check by default, but I can change it to also verify that it is not undefined.
There was a problem hiding this comment.
My question was more around the pattern - property in object vs object.property !== undefined. I am not sure if it makes a difference or if one is better than the other.
packages/runtime/container-runtime/src/test/dataStoreCreation.spec.ts
Outdated
Show resolved
Hide resolved
packages/runtime/container-runtime/src/test/dataStoreContext.spec.ts
Outdated
Show resolved
Hide resolved
packages/runtime/container-runtime/src/test/dataStoreContext.spec.ts
Outdated
Show resolved
Hide resolved
| @@ -337,7 +341,7 @@ describe("Data Store Context Tests", () => { | |||
| it("can correctly initialize and generate attributes", async () => { | |||
| dataStoreAttributes = { | |||
| pkg: JSON.stringify(["TestDataStore1"]), | |||
| snapshotFormatVersion: "0.1", | |||
| summaryFormatVersion: 2, | |||
| isRootDataStore: true, | |||
There was a problem hiding this comment.
I was thinking of tests where these attributes in the initial snapshot for the data store has disableIsolatedChannels as true / false. And make sure we can read and process the snapshot accordingly.
There was a problem hiding this comment.
I've made changes, let me know if this is what you're looking for. It now runs the initialization tests on all 8x combinations.
Read x4: v0, v1, v2, v2 + disableIsolatedChannels
Write x2: v1, v2
packages/runtime/container-runtime/src/test/dataStoreContext.spec.ts
Outdated
Show resolved
Hide resolved
| commits: {}, | ||
| trees: {}, | ||
| }; | ||
| it("reading from latest without isolated channels", async () => testGenerateAttributesCore({ |
There was a problem hiding this comment.
Can you add another test case with disableIsolatedChannels: false. I understand that this attributes not being there means its false. But since this is a boolean and can accept false, we should have a test case to make sure we don't break this scenario.
There was a problem hiding this comment.
It is typed as true | undefined.
There was a problem hiding this comment.
oh okay! I looked at the one in IContainerRuntimeOptions and got confused :-).
* Update lock-files * Expose summarize function which returns tree * Add simple summaries end-to-end test * Increment summary format version and put data stores/dds under .channels subtrees * Fix some tests * Update basic summary test for subtrees * Write in new format in all places * Add runtime option to disable isolated channels * Fix handling initial summary from attach op * Strip out new format version features from snapshots in comparison * Fix all broken tests and places where summaryFormatVersion matters * Final fixes * Update tests * Add back missing skip * Disable by default * UseContainerRuntime for checking if disabled on write; use helper functions when reading * Fix tests * Fix summaries test * Snapshot should always include metadata blob * Random lerna fix * Do not modify snapshots for comparison * Remove extra constructor arg * only expose disableIsolatedChannels instead of entire ContainerRuntime * Point to updated generated snapshot test data * Inline formMetadata calls * Switch datastore attributes to summaryFormatVersion * Use undefined for metadata * Small PR changes * Add a few more tests * Do not write in new format version unless isolated channels are enabled * Revert snapshot test generation * Fix summaries test * Run all 8 combinations for intiialization * PR comments * Fix build
* Update lock-files * Expose summarize function which returns tree * Add simple summaries end-to-end test * Increment summary format version and put data stores/dds under .channels subtrees * Fix some tests * Update basic summary test for subtrees * Write in new format in all places * Add runtime option to disable isolated channels * Fix handling initial summary from attach op * Strip out new format version features from snapshots in comparison * Fix all broken tests and places where summaryFormatVersion matters * Final fixes * Update tests * Add back missing skip * Disable by default * UseContainerRuntime for checking if disabled on write; use helper functions when reading * Fix tests * Fix summaries test * Snapshot should always include metadata blob * Random lerna fix * Do not modify snapshots for comparison * Remove extra constructor arg * only expose disableIsolatedChannels instead of entire ContainerRuntime * Point to updated generated snapshot test data * Inline formMetadata calls * Switch datastore attributes to summaryFormatVersion * Use undefined for metadata * Small PR changes * Add a few more tests * Do not write in new format version unless isolated channels are enabled * Revert snapshot test generation * Fix summaries test * Run all 8 combinations for intiialization * PR comments * Fix build
Fixes #4706 by writing in new format version.
Actually need to leave the writing disabled. It turns out the prior PR to read in the new format was incomplete. Will want to aggressively turn this one as soon as possible.
Also rename some types and file from "snapshot" to "summary", i.e.
snapshot.tsbecomessummaryFormat.ts.