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

Introducing version vector to solve GC problem #899

Merged
merged 38 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
e0988d0
Update Proto
JOOHOJANG Sep 8, 2024
9580b97
Update converter to use version vector
JOOHOJANG Sep 8, 2024
dee2778
Update Document to handle version vector in applychangepack
JOOHOJANG Sep 8, 2024
f6177d2
Define version vector and add into change_id, changepack
JOOHOJANG Sep 8, 2024
df82b83
Update GC in CRDT Root to use min synced version vector
JOOHOJANG Sep 8, 2024
0dd1ece
Update test helper to compute MaxVersionVector
JOOHOJANG Sep 8, 2024
11d4848
Update TC to handle version vector
JOOHOJANG Sep 8, 2024
313a81c
Define version vector getter in document
JOOHOJANG Sep 8, 2024
cadecd8
Fix typo
JOOHOJANG Sep 8, 2024
f4e3431
Define version vector helper in test
JOOHOJANG Sep 8, 2024
3e3afe7
Modify GC test to compare version vector
JOOHOJANG Sep 8, 2024
557702e
Disable empty arrow function lint error
JOOHOJANG Sep 9, 2024
6f383e6
Add GC test for concurrent case and concurrent pushonly case
JOOHOJANG Sep 9, 2024
13f528b
Update bench test
JOOHOJANG Sep 9, 2024
0c6a7db
Temporarily add unload event code to detach user when close/refresh tab
JOOHOJANG Sep 10, 2024
f4f7ff4
Fix lint error
JOOHOJANG Sep 12, 2024
40d5792
Modify buf generate
JOOHOJANG Sep 24, 2024
cae3f9c
Remove snapshot version vector
JOOHOJANG Oct 1, 2024
ee8e411
Remove min synced version vector from proto
JOOHOJANG Oct 1, 2024
e3b438e
Modify min synced version vector related logics
JOOHOJANG Oct 1, 2024
6f14089
Test modifications based on the implementation approach
JOOHOJANG Oct 1, 2024
95e5cac
Remove TODO
JOOHOJANG Oct 22, 2024
047510f
Restrict presence object type to JSON serializable values (#898)
gwbaik9717 Sep 12, 2024
95e8871
Add configurable retry mechanism to broadcast interface (#901)
gwbaik9717 Sep 30, 2024
9f670ad
Ensure `find` and `indexOf` perform splay (#904)
m4ushold Sep 30, 2024
6e98529
Update CHANGELOG.md for v0.5.1 (#910)
hackerwins Oct 15, 2024
6e7e344
Update target to ES2020 and replace Long with bigint (#912)
hackerwins Oct 21, 2024
6376dbf
Define version vector and add into change_id, changepack
JOOHOJANG Sep 8, 2024
06569de
Modify buf generate
JOOHOJANG Sep 24, 2024
73aad7b
Remove snapshot version vector
JOOHOJANG Oct 1, 2024
4dc3e1c
Remove min synced version vector from proto
JOOHOJANG Oct 1, 2024
3aff0b5
Modify min synced version vector related logics
JOOHOJANG Oct 1, 2024
43b2f6b
Merge branch 'main' into hybrid-clock-version-vector-modified
JOOHOJANG Oct 22, 2024
7e39737
Remove Long from helper
JOOHOJANG Oct 22, 2024
3528aa0
Clean up codes
JOOHOJANG Oct 22, 2024
e5321c4
Clean up codes
JOOHOJANG Oct 22, 2024
6d5ece1
Clean up codes
JOOHOJANG Oct 22, 2024
c4dea81
Clean up codes
JOOHOJANG Oct 23, 2024
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
59 changes: 59 additions & 0 deletions packages/sdk/src/api/converter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import { RGATreeList } from '@yorkie-js-sdk/src/document/crdt/rga_tree_list';
import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element';
import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object';
import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array';
import { VersionVector } from '@yorkie-js-sdk/src/document/time/version_vector';
import { CRDTTreePos } from './../document/crdt/tree';
import {
RGATreeSplit,
Expand Down Expand Up @@ -78,6 +79,7 @@ import {
TreeNodes as PbTreeNodes,
TreePos as PbTreePos,
TreeNodeID as PbTreeNodeID,
VersionVector as PbVersionVector,
ValueType as PbValueType,
JSONElement_Tree as PbJSONElement_Tree,
JSONElement_Text as PbJSONElement_Text,
Expand Down Expand Up @@ -161,6 +163,7 @@ function toChangeID(changeID: ChangeID): PbChangeID {
clientSeq: changeID.getClientSeq(),
lamport: changeID.getLamport(),
actorId: toUint8Array(changeID.getActorID()),
versionVector: toVersionVector(changeID.getVersionVector()),
});
}

Expand All @@ -179,6 +182,21 @@ function toTimeTicket(ticket?: TimeTicket): PbTimeTicket | undefined {
});
}

/**
* `toVersionVector` converts the given model to Protobuf format.
*/
function toVersionVector(vector?: VersionVector): PbVersionVector | undefined {
if (!vector) {
return;
}

const pbVector = new PbVersionVector();
for (const [actorID, lamport] of vector) {
pbVector.vector[actorID] = BigInt(lamport.toString());
}
return pbVector;
}

/**
* `toValueType` converts the given model to Protobuf format.
*/
Expand Down Expand Up @@ -780,6 +798,7 @@ function toChangePack(pack: ChangePack<Indexable>): PbChangePack {
isRemoved: pack.getIsRemoved(),
changes: toChanges(pack.getChanges()),
snapshot: pack.getSnapshot(),
versionVector: toVersionVector(pack.getVersionVector()),
minSyncedTicket: toTimeTicket(pack.getMinSyncedTicket()),
});
}
Expand Down Expand Up @@ -810,10 +829,28 @@ function fromChangeID(pbChangeID: PbChangeID): ChangeID {
pbChangeID.clientSeq,
BigInt(pbChangeID.lamport),
toHexString(pbChangeID.actorId),
fromVersionVector(pbChangeID.versionVector)!,
JOOHOJANG marked this conversation as resolved.
Show resolved Hide resolved
BigInt(pbChangeID.serverSeq),
);
}

/**
* `fromVersionVector` converts the given Protobuf format to model format.
*/
function fromVersionVector(
pbVersionVector?: PbVersionVector,
): VersionVector | undefined {
if (!pbVersionVector) {
return;
}

const vector = new VersionVector();
Object.entries(pbVersionVector.vector).forEach(([key, value]) => {
vector.set(key, BigInt(value.toString()));
});
return vector;
}

/**
* `fromTimeTicket` converts the given Protobuf format to model format.
*/
Expand Down Expand Up @@ -1324,6 +1361,7 @@ function fromChangePack<P extends Indexable>(
fromCheckpoint(pbPack.checkpoint!),
pbPack.isRemoved,
fromChanges(pbPack.changes),
fromVersionVector(pbPack.versionVector),
pbPack.snapshot,
fromTimeTicket(pbPack.minSyncedTicket),
);
Expand Down Expand Up @@ -1470,6 +1508,25 @@ function bytesToSnapshot<P extends Indexable>(
};
}

/**
* `versionVectorToHex` converts the given VersionVector to bytes.
*/
function versionVectorToHex(vector: VersionVector): string {
const pbVersionVector = toVersionVector(vector)!;

return bytesToHex(pbVersionVector.toBinary());
}
JOOHOJANG marked this conversation as resolved.
Show resolved Hide resolved

/**
* `hexToVersionVector` creates a VersionVector from the given bytes.
*/
function hexToVersionVector(hex: string): VersionVector {
const bytes = hexToBytes(hex);
const pbVersionVector = PbVersionVector.fromBinary(bytes);

return fromVersionVector(pbVersionVector)!;
}
Comment on lines +1527 to +1528
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid Unnecessary Non-Null Assertion Operator in hexToVersionVector

In the function hexToVersionVector, the non-null assertion operator ! is used in fromVersionVector(pbVersionVector)!. Since fromVersionVector can return undefined, using ! here could result in a runtime error if fromVersionVector returns undefined. It's advisable to handle the potential undefined value or adjust fromVersionVector to guarantee a valid VersionVector is returned.


/**
* `bytesToObject` creates an JSONObject from the given byte array.
*/
Expand Down Expand Up @@ -1602,4 +1659,6 @@ export const converter = {
PbChangeID,
bytesToChangeID,
bytesToOperation,
versionVectorToHex,
hexToVersionVector,
};
6 changes: 6 additions & 0 deletions packages/sdk/src/api/yorkie/v1/resources.proto
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ message ChangePack {
repeated Change changes = 4;
TimeTicket min_synced_ticket = 5;
bool is_removed = 6;
VersionVector version_vector = 7;
}

message Change {
Expand All @@ -60,6 +61,11 @@ message ChangeID {
int64 server_seq = 2;
int64 lamport = 3;
bytes actor_id = 4;
VersionVector version_vector = 5;
}

message VersionVector {
map<string, int64> vector = 1;
}

message Operation {
Expand Down
49 changes: 49 additions & 0 deletions packages/sdk/src/api/yorkie/v1/resources_pb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,11 @@ export class ChangePack extends Message<ChangePack> {
*/
isRemoved = false;

/**
* @generated from field: yorkie.v1.VersionVector version_vector = 7;
*/
versionVector?: VersionVector;

constructor(data?: PartialMessage<ChangePack>) {
super();
proto3.util.initPartial(data, this);
Expand All @@ -243,6 +248,7 @@ export class ChangePack extends Message<ChangePack> {
{ no: 4, name: "changes", kind: "message", T: Change, repeated: true },
{ no: 5, name: "min_synced_ticket", kind: "message", T: TimeTicket },
{ no: 6, name: "is_removed", kind: "scalar", T: 8 /* ScalarType.BOOL */ },
{ no: 7, name: "version_vector", kind: "message", T: VersionVector },
]);

static fromBinary(bytes: Uint8Array, options?: Partial<BinaryReadOptions>): ChangePack {
Expand Down Expand Up @@ -341,6 +347,11 @@ export class ChangeID extends Message<ChangeID> {
*/
actorId = new Uint8Array(0);

/**
* @generated from field: yorkie.v1.VersionVector version_vector = 5;
*/
versionVector?: VersionVector;

constructor(data?: PartialMessage<ChangeID>) {
super();
proto3.util.initPartial(data, this);
Expand All @@ -353,6 +364,7 @@ export class ChangeID extends Message<ChangeID> {
{ no: 2, name: "server_seq", kind: "scalar", T: 3 /* ScalarType.INT64 */ },
{ no: 3, name: "lamport", kind: "scalar", T: 3 /* ScalarType.INT64 */ },
{ no: 4, name: "actor_id", kind: "scalar", T: 12 /* ScalarType.BYTES */ },
{ no: 5, name: "version_vector", kind: "message", T: VersionVector },
]);

static fromBinary(bytes: Uint8Array, options?: Partial<BinaryReadOptions>): ChangeID {
Expand All @@ -372,6 +384,43 @@ export class ChangeID extends Message<ChangeID> {
}
}

/**
* @generated from message yorkie.v1.VersionVector
*/
export class VersionVector extends Message<VersionVector> {
/**
* @generated from field: map<string, int64> vector = 1;
*/
vector: { [key: string]: bigint } = {};

constructor(data?: PartialMessage<VersionVector>) {
super();
proto3.util.initPartial(data, this);
}

static readonly runtime: typeof proto3 = proto3;
static readonly typeName = "yorkie.v1.VersionVector";
static readonly fields: FieldList = proto3.util.newFieldList(() => [
{ no: 1, name: "vector", kind: "map", K: 9 /* ScalarType.STRING */, V: {kind: "scalar", T: 3 /* ScalarType.INT64 */} },
]);

static fromBinary(bytes: Uint8Array, options?: Partial<BinaryReadOptions>): VersionVector {
return new VersionVector().fromBinary(bytes, options);
}

static fromJson(jsonValue: JsonValue, options?: Partial<JsonReadOptions>): VersionVector {
return new VersionVector().fromJson(jsonValue, options);
}

static fromJsonString(jsonString: string, options?: Partial<JsonReadOptions>): VersionVector {
return new VersionVector().fromJsonString(jsonString, options);
}

static equals(a: VersionVector | PlainMessage<VersionVector> | undefined, b: VersionVector | PlainMessage<VersionVector> | undefined): boolean {
return proto3.util.equals(VersionVector, a, b);
}
}
JOOHOJANG marked this conversation as resolved.
Show resolved Hide resolved

/**
* @generated from message yorkie.v1.Operation
*/
Expand Down
93 changes: 79 additions & 14 deletions packages/sdk/src/document/change/change_id.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,35 @@ import {
InitialActorID,
} from '@yorkie-js-sdk/src/document/time/actor_id';
import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket';
import { InitialVersionVector, VersionVector } from '../time/version_vector';

/**
* `ChangeID` is for identifying the Change. This is immutable.
*/
export class ChangeID {
// `clientSeq` is the sequence number of the client that created this change.
private clientSeq: number;

// `serverSeq` is optional and only present for changes stored on the server.
private serverSeq?: bigint;

// `lamport` and `actor` are the lamport clock and the actor of this change.
// This is used to determine the order of changes in logical time.
private lamport: bigint;
private actor: ActorID;
// `versionVector` is the vector clock of this change. This is used to
// determine the relationship is causal or not between changes.
private versionVector: VersionVector;

constructor(
clientSeq: number,
lamport: bigint,
actor: ActorID,
vector: VersionVector,
serverSeq?: bigint,
) {
this.clientSeq = clientSeq;
this.serverSeq = serverSeq;
this.lamport = lamport;
this.versionVector = vector;
this.actor = actor;
}

Expand All @@ -51,29 +58,56 @@ export class ChangeID {
clientSeq: number,
lamport: bigint,
actor: ActorID,
vector: VersionVector,
serverSeq?: bigint,
): ChangeID {
return new ChangeID(clientSeq, lamport, actor, serverSeq);
return new ChangeID(clientSeq, lamport, actor, vector, serverSeq);
}

/**
* `next` creates a next ID of this ID.
*/
public next(): ChangeID {
return new ChangeID(this.clientSeq + 1, this.lamport + 1n, this.actor);
const vector = this.versionVector.deepcopy();
vector.set(this.actor, this.lamport + 1n);

return new ChangeID(
this.clientSeq + 1,
this.lamport + 1n,
this.actor,
vector,
);
}

/**
* `syncLamport` syncs lamport timestamp with the given ID.
*
* {@link https://en.wikipedia.org/wiki/Lamport_timestamps#Algorithm}
* `syncClocks` syncs logical clocks with the given ID.
*/
public syncLamport(otherLamport: bigint): ChangeID {
if (otherLamport > this.lamport) {
return new ChangeID(this.clientSeq, otherLamport, this.actor);
}
public syncClocks(other: ChangeID): ChangeID {
const lamport =
other.lamport > this.lamport ? other.lamport + 1n : this.lamport + 1n;
Comment on lines +86 to +87
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct lamport calculation in syncClocks method

In the syncClocks method, the lamport clock calculation might introduce an off-by-one error:

const lamport = other.lamport > this.lamport ? other.lamport + 1n : this.lamport + 1n;

According to Lamport clock synchronization rules, the new lamport should be max(this.lamport, other.lamport) + 1n. The current implementation adds 1n to the larger lamport value, potentially leading to an incorrect increment when other.lamport is greater. Consider revising the calculation:

-const lamport = other.lamport > this.lamport ? other.lamport + 1n : this.lamport + 1n;
+const lamport = (other.lamport > this.lamport ? other.lamport : this.lamport) + 1n;

const maxVersionVector = this.versionVector.max(other.versionVector);

const newID = new ChangeID(
this.clientSeq,
lamport,
this.actor,
maxVersionVector,
);
newID.versionVector.set(this.actor, lamport);
return newID;
}

return new ChangeID(this.clientSeq, this.lamport + 1n, this.actor);
/**
* `setClocks` sets the given clocks to this ID. This is used when the snapshot
* is given from the server.
*/
public setClocks(otherLamport: bigint, vector: VersionVector): ChangeID {
const lamport =
otherLamport > this.lamport ? otherLamport : this.lamport + 1n;
Comment on lines +105 to +106
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Adjust lamport calculation in setClocks method

In the setClocks method, the lamport clock calculation may not increment correctly:

const lamport = otherLamport > this.lamport ? otherLamport : this.lamport + 1n;

To adhere to Lamport clock rules, the lamport should be set to max(this.lamport, otherLamport) + 1n. This ensures the clock advances properly regardless of which lamport is greater. Consider updating the calculation:

-const lamport = otherLamport > this.lamport ? otherLamport : this.lamport + 1n;
+const lamport = (otherLamport > this.lamport ? otherLamport : this.lamport) + 1n;

const maxVersionVector = this.versionVector.max(vector);
maxVersionVector.set(this.actor, lamport);

return ChangeID.of(this.clientSeq, lamport, this.actor, maxVersionVector);
}

/**
Expand All @@ -87,7 +121,26 @@ export class ChangeID {
* `setActor` sets the given actor.
*/
public setActor(actorID: ActorID): ChangeID {
return new ChangeID(this.clientSeq, this.lamport, actorID, this.serverSeq);
return new ChangeID(
this.clientSeq,
this.lamport,
actorID,
this.versionVector,
this.serverSeq,
);
}

/**
* `setVersionVector` sets the given version vector.
*/
public setVersionVector(versionVector: VersionVector): ChangeID {
return new ChangeID(
this.clientSeq,
this.lamport,
this.actor,
versionVector,
this.serverSeq,
);
}

/**
Expand Down Expand Up @@ -128,6 +181,13 @@ export class ChangeID {
return this.actor;
}

/**
* `getVersionVector` returns the version vector of this ID.
*/
public getVersionVector(): VersionVector {
return this.versionVector;
}

/**
* `toTestString` returns a string containing the meta data of this ID.
*/
Expand All @@ -142,4 +202,9 @@ export class ChangeID {
* `InitialChangeID` represents the initial state ID. Usually this is used to
* represent a state where nothing has been edited.
*/
export const InitialChangeID = new ChangeID(0, 0n, InitialActorID);
export const InitialChangeID = new ChangeID(
0,
0n,
InitialActorID,
InitialVersionVector,
);
Loading
Loading