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

Document Trailing Ops Compatibility #11685

Merged
merged 6 commits into from Aug 30, 2022

Conversation

CraigMacomber
Copy link
Contributor

Description

Add DDS readme covering trailing ops

Reviewer Guidance

I'd like feedback on the documentation itself, as well as if this is a good place for it.

@CraigMacomber CraigMacomber requested a review from a team as a code owner August 25, 2022 20:30
@github-actions github-actions bot added area: dds Issues related to distributed data structures base: main PRs targeted against main branch labels Aug 25, 2022
packages/dds/README.md Outdated Show resolved Hide resolved
@alexvy86
Copy link
Contributor

It took me a few reads to fully understand this part:

"Since Fluid supports being used in a mode (and it typically used in this mode) where there is no service that summarizes documents to bake these trailing ops into the DDS level summaries...

At first I got the impression that "the DDS level summaries" were a separate kind of summary. But if I ended up understanding it correctly, you mean "on the server/service side, there's nothing that transforms summary + trailing ops into new summary that includes the trailing ops", right?

Regarding the location, this one seems fine to me.

Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

As a DDS implementer who is probably not as informed about this area as I should be, thank you for writing this :-P

Comment on lines 11 to 12
When initializing a DDS instance from an existing document, first the summary is loaded, and then any ops since that summary are applied.
These ops are called "Trailing Ops", and they can get persisted as part of the server side summarization when a session ends between summaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this reads easier if these sentences are flipped to explain what the trailing ops are before explaining their role in load, i.e. something like "When a session ends between summaries, the most recent ops may not be included in the latest summary. These ops are still persisted by the service as "trailing ops". When a DDS is loaded, first the summary is loaded and then the trailing ops are applied."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that order is better. I have rephrased accordingly.

These ops are called "Trailing Ops", and they can get persisted as part of the server side summarization when a session ends between summaries.
Since Fluid supports being used in a mode (and it typically used in this mode) where there is no service that summarizes documents to bake these trailing ops into the DDS level summaries,
these trailing ops can live in documents for unbounded amounts of time.
This means DDSes should generally support processing all past versions of ops that were ever used (in addition to supporting all past summary formats) so that old documents continue to be openable.
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 required regardless of whether the trailing ops are in the summary vs. delta storage, right? Either way op back-compat is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what "delta storage" means in this context, regardless we do require op back compat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the new phrasing is now less specific about how the data is stored, so this distinction likley doesn't matter much anymore.


When initializing a DDS instance from an existing document, first the summary is loaded, and then any ops since that summary are applied.
These ops are called "Trailing Ops", and they can get persisted as part of the server side summarization when a session ends between summaries.
Since Fluid supports being used in a mode (and it typically used in this mode) where there is no service that summarizes documents to bake these trailing ops into the DDS level summaries,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you looking to draw a distinction here about the mechanism used to store the trailing ops? If so, maybe call out the possibilities more explicitly, e.g. "Fluid supports two ways a service might store trailing ops: either as explicit deltas using DeltaStorage, or included as part of the summary itself".

Also -- this document states it is for DDS implementers -- what do they need to know about this nuance? Seems like this all happens behind the scenes and a DDS implementer doesn't care?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, the trailing ops could be stored still as independent ops within a summary? I thought that "baked into the summary" meant "they are already considered when generating the snapshot that is the latest summary", which to me meant that at that point the ops as separate entities did not exist anymore (or at least weren't relevant anymore). If the former is true, then I guess I'm an example that making that a bit more explicit could be good 😄 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have rephased this to hopefully be clearer.

I have no idea what DeltaStorage is, so I'm not addressing it here. It seems unnecessary for making the point that we need back compat. If you understand it, and think it would be useful to cover in this document, feel free to add it :).

@ChumpChief
Copy link
Contributor

I'd like feedback on the documentation itself, as well as if this is a good place for it.

Since this is under /packages/dds, maybe also consider stubbing out headers/structure for what else you think would be useful here (kind of a wikipedia red-linking strategy). In particular, to clarify whether the content is meant to only serve DDS implementers, or also DDS users. Seems fine to me though.

CraigMacomber and others added 4 commits August 29, 2022 15:56
Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
@CraigMacomber
Copy link
Contributor Author

I'd like feedback on the documentation itself, as well as if this is a good place for it.

Since this is under /packages/dds, maybe also consider stubbing out headers/structure for what else you think would be useful here (kind of a wikipedia red-linking strategy). In particular, to clarify whether the content is meant to only serve DDS implementers, or also DDS users. Seems fine to me though.

Thats sounds good. I'll leave it for a separate PR though as I can imagine a good amount of discussion about what all belongs in here.

packages/dds/README.md Outdated Show resolved Hide resolved
Co-authored-by: Matt Rakow <ChumpChief@users.noreply.github.com>
@CraigMacomber CraigMacomber merged commit 8a4f5c9 into microsoft:main Aug 30, 2022
@CraigMacomber CraigMacomber deleted the trailingDocs branch August 30, 2022 22:55
@github-actions
Copy link
Contributor

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

WayneFerrao pushed a commit to WayneFerrao/FluidFramework that referenced this pull request Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds Issues related to distributed data structures base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants