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

Fixes to eventual consistency of shared intervals, including significant LocalReference changes #10202

Merged
merged 75 commits into from Jun 3, 2022

Conversation

ransomr
Copy link
Contributor

@ransomr ransomr commented May 9, 2022

Implement new MergeTree LocalReference APIs to fix eventual consistency of SlideOnRemove LocalReferences.
Update SharedIntervals to use new APIs.
For more detailed documentation, see packages/dds/merge-tree/REFERENCEPOSITIONS.md

@ransomr ransomr requested review from msfluid-bot and a team as code owners May 9, 2022 13:51
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: sharedstring area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct dependencies Pull requests that update a dependency file public api change Changes to a public API labels May 9, 2022
@github-actions github-actions bot added the base: main PRs targeted against main branch label May 9, 2022
@Abe27342
Copy link
Contributor

Looked at tests, not too many comments there. Obviously there are still some TODOs, but I'm ok with addressing them in follow-ups.

@Abe27342 Abe27342 requested a review from DLehenbauer May 25, 2022 16:05
@Abe27342
Copy link
Contributor

What's the rationale for the explicit update of reference offsets on segment removal? I don't think we can come up with an eventually consistent scheme while doing that. If it's just to make sure the observed offset matches the segment's position in the SharedString, there is already logic which handles returning 0 in getOffset() when the segment is removed.

Here's an explicit fuzz test case (minimized, so pretty readable) which demonstrates why this strategy loses information:

[
    {
        "type": "addText",
        "index": 0,
        "content": "ABCDEFGHIJ",
        "stringId": "B"
    },
    {
        "type": "synchronize"
    },
    {
        "type": "addInterval",
        "start": 6,
        "end": 8,
        "collectionName": "comments",
        "stringId": "B",
        "id": "7a5d683d-4b3e-44b3-8f91-e8f9d664de5a"
    },
    {
        "type": "addText",
        "index": 7,
        "content": "XYZ",
        "stringId": "B"
    },
    {
        "type": "removeRange",
        "start": 2,
        "end": 10,
        "stringId": "A"
    },
    {
        "type": "synchronize"
    }
]

A needs to be able to slide the references added by B to the start and end of XYZ, respectively, but it won't know how to do this if it blindly sets both offsets to 0 upon acking them.

(side note: I do think we will probably need to either expose a method which gets the offset ignoring segment removals, or push reference comparison + "split" knowledge somehow to the reference itself. There are a couple maintenance things which we do need to make sure we preserve that information)

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jun 2, 2022

@fluid-example/bundle-size-tests: +6.74 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 392.54 KB 395.14 KB +2.6 KB
connectionState.js 711 Bytes 711 Bytes No change
containerRuntime.js 211.42 KB 211.42 KB No change
loader.js 146.35 KB 146.35 KB No change
map.js 38.3 KB 38.3 KB No change
matrix.js 121.7 KB 123.15 KB +1.45 KB
odspDriver.js 144.89 KB 144.89 KB No change
odspPrefetchSnapshot.js 36.83 KB 36.83 KB No change
sharedString.js 138.32 KB 141.01 KB +2.69 KB
Total Size 1.23 MB 1.24 MB +6.74 KB

Baseline commit: f01e36f

Generated by 🚫 dangerJS against a7651f8

Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

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

REFERENCEPOSITIONS.md could use another once-over, but generally this looks great. Thanks for all your hard work on it :)

@ransomr ransomr merged commit 97c79d4 into microsoft:main Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: sharedstring area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants