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

Merge-Tree and SharedString ISegment Deprecations #23323

Merged

Conversation

anthony-murphy
Copy link
Contributor

@anthony-murphy anthony-murphy commented Dec 13, 2024

The current ISegment interface over-exposes a number of properties which do not have an external use case, and any external usage could result in damage to the underlying merge-tree including data corruption.

The only use case that will continue to be supported is determining if a segment is removed. For this purpose we've add the following function segmentIsRemoved(segment: ISegment): boolean

For example, checking if a segment is not removed would change as follows:

- if(segment.removedSeq === undefined){
+ if(!segmentIsRemoved(segment)){

The following properties are deprecated on ISegment and its implementations:

  • clientId
  • index
  • localMovedSeq
  • localRefs
  • localRemovedSeq
  • localSeq
  • movedClientsIds
  • movedSeq
  • movedSeqs
  • ordinal
  • removedClientIds
  • removedSeq
  • seq
  • wasMovedOnInsert

Additionally, the following types are also deprecated, and will become internal:

  • IMergeNodeCommon
  • IMoveInfo
  • IRemovalInfo
  • LocalReferenceCollection

@Copilot Copilot bot review requested due to automatic review settings December 13, 2024 00:33
@anthony-murphy anthony-murphy requested review from a team as code owners December 13, 2024 00:33

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: examples Changes that focus on our examples changeset-present public api change Changes to a public API base: main PRs targeted against main branch labels Dec 13, 2024
@anthony-murphy anthony-murphy force-pushed the deprecate-isegment-properties branch from 5225291 to 59051de Compare December 13, 2024 01:38
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

Aproving for docs with a couple of suggestions.

.changeset/real-grapes-kick.md Outdated Show resolved Hide resolved
.changeset/real-grapes-kick.md Outdated Show resolved Hide resolved
anthony-murphy and others added 2 commits December 13, 2024 08:53
Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com>
Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>
@anthony-murphy anthony-murphy enabled auto-merge (squash) December 13, 2024 19:07
Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170006 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors


@anthony-murphy anthony-murphy merged commit e8762e3 into microsoft:main Dec 13, 2024
30 checks passed
anthony-murphy added a commit that referenced this pull request Dec 20, 2024
)

This change aligns the insertion information of an ISegment with the
existing IRemovalInfo and IMoveInfo by adding a new interface,
IInsertionInfo. This work is primarily precursor work, which is related
to #23323, as these changes were made with some of those deprecation's
removed locally so that we can reduce the eventual churn when we finally
remove those properties.

In addition to adding the new interface, this change also standardizes
the methods we provide for each of these interfaces;
- toInsertionInfo/toRemovealInfo/toMoveInfo
- isInserted/isRemoved/IsMoved
- assertInserted/assertRemoved/assertMoved

There are also some general utility types and methods which make the
info types easier to work with:
- SegmentInfo
- SegmentWithInfo
- overwriteSegmentInfo
After deprecations are removed. These types will also aid in improving
typing to reduce the need for assertions.

Lastly, I moved all the info from ISegmentInternal to ISegmentLeaf. This
better approximates the final state, and allow further reducing the
exposure of the internal state of the segment. Due to this some changes
in other packages were necessary, but all could be solved with the
existing segmentIsRemovedFunction

---------

Co-authored-by: jzaffiro <110866475+jzaffiro@users.noreply.github.com>
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 area: examples Changes that focus on our examples base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants