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

improvement(client): deprecation cleanup #22948

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/dds/matrix/src/test/matrix.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
IChannelServices,
type IChannel,
} from "@fluidframework/datastore-definitions/internal";
import type { ISegmentInternal } from "@fluidframework/merge-tree/internal";
import {
MockContainerRuntimeFactory,
MockContainerRuntimeFactoryForReconnection,
Expand Down Expand Up @@ -967,7 +968,7 @@ describe("Matrix1", () => {

function findVectorReferenceCount(vector: PermutationVector): number {
let count = 0;
vector.walkSegments((segment) => {
vector.walkSegments((segment: ISegmentInternal) => {
count += [...(segment.localRefs ?? [])].length;
return true;
});
Expand Down
1 change: 1 addition & 0 deletions packages/dds/merge-tree/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export {
AttributionPolicy,
IMergeTreeAttributionOptions,
IMergeTreeOptions,
IMergeTreeOptionsInternal,
getSlideToSegoff,
} from "./mergeTree.js";
export {
Expand Down
6 changes: 1 addition & 5 deletions packages/dds/merge-tree/src/localReference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { assert } from "@fluidframework/core-utils/internal";
import { UsageError } from "@fluidframework/telemetry-utils/internal";

import { DoublyLinkedList, ListNode, walkList } from "./collections/index.js";
import { ISegmentInternal, type ISegment } from "./mergeTreeNodes.js";
import type { ISegmentInternal, ISegment } from "./mergeTreeNodes.js";
import { TrackingGroup, TrackingGroupCollection } from "./mergeTreeTracking.js";
import { ReferenceType } from "./ops.js";
import { PropertySet, addProperties } from "./properties.js";
Expand Down Expand Up @@ -79,10 +79,6 @@ export interface LocalReferencePosition extends ReferencePosition {
addProperties(newProps: PropertySet): void;
}

/**
* @privateRemarks This should not be exported outside merge tree.
* @internal
*/
class LocalReference implements LocalReferencePosition {
public properties: PropertySet | undefined;

Expand Down
80 changes: 78 additions & 2 deletions packages/dds/merge-tree/src/mergeTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,71 @@ const LRUSegmentComparer: IComparer<LRUSegment> = {
compare: (a, b) => a.maxSeq - b.maxSeq,
};

export function ackSegment(
segment: ISegmentLeaf,
// eslint-disable-next-line import/no-deprecated
segmentGroup: SegmentGroup,
opArgs: IMergeTreeDeltaOpArgs,
): boolean {
const currentSegmentGroup = segment.segmentGroups?.dequeue();
assert(currentSegmentGroup === segmentGroup, 0x043 /* "On ack, unexpected segmentGroup!" */);
switch (opArgs.op.type) {
case MergeTreeDeltaType.ANNOTATE: {
assert(
!!segment.propertyManager,
0x044 /* "On annotate ack, missing segment property manager!" */,
);
segment.propertyManager.ackPendingProperties(opArgs.op);
return true;
}

case MergeTreeDeltaType.INSERT: {
assert(
segment.seq === UnassignedSequenceNumber,
0x045 /* "On insert, seq number already assigned!" */,
);
segment.seq = opArgs.sequencedMessage!.sequenceNumber;
segment.localSeq = undefined;
return true;
}

case MergeTreeDeltaType.REMOVE: {
const removalInfo: IRemovalInfo | undefined = toRemovalInfo(segment);
assert(removalInfo !== undefined, 0x046 /* "On remove ack, missing removal info!" */);
segment.localRemovedSeq = undefined;
if (removalInfo.removedSeq === UnassignedSequenceNumber) {
removalInfo.removedSeq = opArgs.sequencedMessage!.sequenceNumber;
return true;
}
return false;
}

case MergeTreeDeltaType.OBLITERATE:
case MergeTreeDeltaType.OBLITERATE_SIDED: {
const moveInfo: IMoveInfo | undefined = toMoveInfo(segment);
assert(moveInfo !== undefined, 0x86e /* On obliterate ack, missing move info! */);
const obliterateInfo = segmentGroup.obliterateInfo;
assert(obliterateInfo !== undefined, 0xa40 /* must have obliterate info */);
segment.localMovedSeq = obliterateInfo.localSeq = undefined;
const seqIdx = moveInfo.movedSeqs.indexOf(UnassignedSequenceNumber);
assert(seqIdx !== -1, 0x86f /* expected movedSeqs to contain unacked seq */);
moveInfo.movedSeqs[seqIdx] = obliterateInfo.seq =
opArgs.sequencedMessage!.sequenceNumber;

if (moveInfo.movedSeq === UnassignedSequenceNumber) {
moveInfo.movedSeq = opArgs.sequencedMessage!.sequenceNumber;
return true;
}

return false;
}

default: {
throw new Error(`${opArgs.op.type} is in unrecognized operation type`);
}
}
}

/**
* @legacy
* @alpha
Expand Down Expand Up @@ -212,6 +277,17 @@ export interface IMergeTreeOptions {
*/
mergeTreeEnableSidedObliterate?: boolean;
}

/**
* @internal
*/
export interface IMergeTreeOptionsInternal extends IMergeTreeOptions {
/**
* Options related to attribution
*/
attribution?: IMergeTreeAttributionOptions;
}

export function errorIfOptionNotTrue(
options: IMergeTreeOptions | undefined,
option: keyof IMergeTreeOptions,
Expand Down Expand Up @@ -527,7 +603,7 @@ export class MergeTree {

private readonly obliterates = new Obliterates(this);

public constructor(public options?: IMergeTreeOptions) {
public constructor(public options?: IMergeTreeOptionsInternal) {
this._root = this.makeBlock(0);
this._root.mergeTree = this;
this.attributionPolicy = options?.attribution?.policyFactory?.();
Expand Down Expand Up @@ -1234,7 +1310,7 @@ export class MergeTree {
const deltaSegments: IMergeTreeSegmentDelta[] = [];
const overlappingRemoves: boolean[] = [];
pendingSegmentGroup.segments.map((pendingSegment: ISegmentLeaf) => {
const overlappingRemove = !pendingSegment.ack(pendingSegmentGroup, opArgs);
const overlappingRemove = !ackSegment(pendingSegment, pendingSegmentGroup, opArgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

i didn't do this initially on purpose, as there was/is active development in the area, and i'm trying to not clobber other people's work


overwrite = overlappingRemove || overwrite;

Expand Down
69 changes: 4 additions & 65 deletions packages/dds/merge-tree/src/mergeTreeNodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ import {
UniversalSequenceNumber,
} from "./constants.js";
import { LocalReferenceCollection, type LocalReferencePosition } from "./localReference.js";
import { ackSegment } from "./mergeTree.js";
import { IMergeTreeDeltaOpArgs } from "./mergeTreeDeltaCallback.js";
import { TrackingGroupCollection } from "./mergeTreeTracking.js";
import { IJSONSegment, IMarkerDef, MergeTreeDeltaType, ReferenceType } from "./ops.js";
import { IJSONSegment, IMarkerDef, ReferenceType } from "./ops.js";
import { computeHierarchicalOrdinal } from "./ordinal.js";
import type { PartialSequenceLengths } from "./partialLengths.js";
import { PropertySet, clone, createMap, type MapLike } from "./properties.js";
Expand Down Expand Up @@ -77,7 +78,6 @@ export type ISegmentLeaf = ISegmentInternal & {
segmentGroups?: SegmentGroupCollection;
// eslint-disable-next-line import/no-deprecated
propertyManager?: PropertiesManager;

/**
* If a segment is inserted into an obliterated range,
* but the newest obliteration of that range was by the inserting client,
Expand Down Expand Up @@ -265,7 +265,6 @@ export interface ISegment extends IMergeNodeCommon, Partial<IRemovalInfo>, Parti
* This is defined if and only if the insertion of the segment is pending ack, i.e. `seq` is UnassignedSequenceNumber.
* Once the segment is acked, this field is cleared.
*
* See {@link CollaborationWindow.localSeq} for more information on the semantics of localSeq.
*/
localSeq?: number;
/**
Expand All @@ -274,7 +273,6 @@ export interface ISegment extends IMergeNodeCommon, Partial<IRemovalInfo>, Parti
* will be updated to the seq at which that client removed this segment.
*
* Like {@link ISegment.localSeq}, this field is cleared once the local removal of the segment is acked.
* See {@link CollaborationWindow.localSeq} for more information on the semantics of localSeq.
*/
localRemovedSeq?: number;
/**
Expand Down Expand Up @@ -386,7 +384,7 @@ export interface BlockAction<TClientData> {
export interface NodeAction<TClientData> {
// eslint-disable-next-line @typescript-eslint/prefer-function-type
(
node: MergeNode,
node: IMergeNode,
pos: number,
refSeq: number,
clientId: number,
Expand Down Expand Up @@ -658,66 +656,7 @@ export abstract class BaseSegment implements ISegment {
* @deprecated - This function should not be used externally and will be removed in a subsequent release.
*/
public ack(segmentGroup: SegmentGroup, opArgs: IMergeTreeDeltaOpArgs): boolean {
const currentSegmentGroup = this.segmentGroups.dequeue();
assert(
currentSegmentGroup === segmentGroup,
0x043 /* "On ack, unexpected segmentGroup!" */,
);
switch (opArgs.op.type) {
case MergeTreeDeltaType.ANNOTATE: {
assert(
!!this.propertyManager,
0x044 /* "On annotate ack, missing segment property manager!" */,
);
this.propertyManager.ackPendingProperties(opArgs.op);
return true;
}

case MergeTreeDeltaType.INSERT: {
assert(
this.seq === UnassignedSequenceNumber,
0x045 /* "On insert, seq number already assigned!" */,
);
this.seq = opArgs.sequencedMessage!.sequenceNumber;
this.localSeq = undefined;
return true;
}

case MergeTreeDeltaType.REMOVE: {
const removalInfo: IRemovalInfo | undefined = toRemovalInfo(this);
assert(removalInfo !== undefined, 0x046 /* "On remove ack, missing removal info!" */);
this.localRemovedSeq = undefined;
if (removalInfo.removedSeq === UnassignedSequenceNumber) {
removalInfo.removedSeq = opArgs.sequencedMessage!.sequenceNumber;
return true;
}
return false;
}

case MergeTreeDeltaType.OBLITERATE:
case MergeTreeDeltaType.OBLITERATE_SIDED: {
const moveInfo: IMoveInfo | undefined = toMoveInfo(this);
assert(moveInfo !== undefined, 0x86e /* On obliterate ack, missing move info! */);
const obliterateInfo = segmentGroup.obliterateInfo;
assert(obliterateInfo !== undefined, 0xa40 /* must have obliterate info */);
this.localMovedSeq = obliterateInfo.localSeq = undefined;
const seqIdx = moveInfo.movedSeqs.indexOf(UnassignedSequenceNumber);
assert(seqIdx !== -1, 0x86f /* expected movedSeqs to contain unacked seq */);
moveInfo.movedSeqs[seqIdx] = obliterateInfo.seq =
opArgs.sequencedMessage!.sequenceNumber;

if (moveInfo.movedSeq === UnassignedSequenceNumber) {
moveInfo.movedSeq = opArgs.sequencedMessage!.sequenceNumber;
return true;
}

return false;
}

default: {
throw new Error(`${opArgs.op.type} is in unrecognized operation type`);
}
}
return ackSegment(this, segmentGroup, opArgs);
}

public splitAt(pos: number): ISegment | undefined {
Expand Down
3 changes: 0 additions & 3 deletions packages/dds/merge-tree/src/mergeTreeTracking.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import { LocalReferencePosition } from "./localReference.js";
import { ISegment } from "./mergeTreeNodes.js";
// eslint-disable-next-line import/no-deprecated
import { SortedSegmentSet } from "./sortedSegmentSet.js";

/**
Expand All @@ -31,11 +30,9 @@ export interface ITrackingGroup {
* @alpha
*/
export class TrackingGroup implements ITrackingGroup {
// eslint-disable-next-line import/no-deprecated
private readonly trackedSet: SortedSegmentSet<Trackable>;

constructor() {
// eslint-disable-next-line import/no-deprecated
this.trackedSet = new SortedSegmentSet<Trackable>();
}

Expand Down
2 changes: 0 additions & 2 deletions packages/dds/merge-tree/src/partialLengths.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,8 @@ import {
toRemovalInfo,
type MergeBlock,
} from "./mergeTreeNodes.js";
// eslint-disable-next-line import/no-deprecated
import { SortedSet } from "./sortedSet.js";

// eslint-disable-next-line import/no-deprecated
class PartialSequenceLengthsSet extends SortedSet<PartialSequenceLength, number> {
protected getKey(item: PartialSequenceLength): number {
return item.seq;
Expand Down
4 changes: 1 addition & 3 deletions packages/dds/merge-tree/src/sortedSegmentSet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@

import { LocalReferencePosition } from "./localReference.js";
import { ISegment } from "./mergeTreeNodes.js";
// eslint-disable-next-line import/no-deprecated
import { SortedSet } from "./sortedSet.js";

/**
* @deprecated This functionality was not meant to be exported and will be removed in a future release
* @internal
*/
export type SortedSegmentSetItem =
Expand All @@ -29,7 +27,7 @@ export type SortedSegmentSetItem =
*
* @internal
*/
// eslint-disable-next-line import/no-deprecated

export class SortedSegmentSet<T extends SortedSegmentSetItem = ISegment> extends SortedSet<
T,
string
Expand Down
1 change: 0 additions & 1 deletion packages/dds/merge-tree/src/sortedSet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/

/**
* @deprecated This functionality was not meant to be exported and will be removed in a future release
* @internal
*/
export abstract class SortedSet<T, U extends string | number> {
Expand Down
9 changes: 6 additions & 3 deletions packages/dds/merge-tree/src/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,12 @@ export {
runMergeTreeOperationRunner,
TestOperation,
} from "./mergeTreeOperationRunner.js";
export { LRUSegment, MergeTree } from "../mergeTree.js";
export {
LRUSegment,
MergeTree,
IMergeTreeOptions,
IMergeTreeOptionsInternal,
} from "../mergeTree.js";
export { MergeTreeTextHelper } from "../MergeTreeTextHelper.js";
export { SnapshotLegacy } from "../snapshotlegacy.js";
export {
Expand Down Expand Up @@ -90,7 +95,6 @@ export {
Marker,
matchProperties,
maxReferencePosition,
MergeNode,
MergeTreeDeltaOperationType,
MergeTreeDeltaOperationTypes,
MergeTreeDeltaRevertible,
Expand All @@ -116,7 +120,6 @@ export {
reservedTileLabelsKey,
revertMergeTreeDeltaRevertibles,
SegmentGroup,
SegmentGroupCollection,
SortedSegmentSet,
SortedSegmentSetItem,
SortedSet,
Expand Down
6 changes: 3 additions & 3 deletions packages/dds/merge-tree/src/test/mergeTree.annotate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ describe("MergeTree", () => {
currentSequenceNumber,
localClientId,
);
assert(segmentInfo.segment?.segmentGroups?.empty);
assert(segmentInfo.segment?.segmentGroups?.empty !== false);

mergeTree.annotateRange(
annotateStart,
Expand All @@ -627,7 +627,7 @@ describe("MergeTree", () => {
undefined as never,
);

assert.equal(segmentInfo.segment?.segmentGroups.size, 1);
assert.equal(segmentInfo.segment?.segmentGroups?.size, 1);

mergeTree.ackPendingSegment({
op: {
Expand All @@ -641,7 +641,7 @@ describe("MergeTree", () => {
} as unknown as ISequencedDocumentMessage,
});

assert(segmentInfo.segment?.segmentGroups.empty);
assert(segmentInfo.segment?.segmentGroups?.empty);
assert.equal(segmentInfo.segment?.properties?.propertySource, "local");
assert.equal(segmentInfo.segment?.properties?.remoteProperty, 1);
});
Expand Down
Loading
Loading