Remove duplicated version vector from pack (#1024)

Unify VersionVectors in ChangePack across three scenarios: 1. Pushing pack to server: represents document's current version vector 2. Pulling pack: represents minSyncedVersionVector for GC 3. Pulling pack(snapshot): represents snapshot's version vector at creation This commit simplifies the codebase and ensures consistent version vector handling throughout the document synchronization process.
yorkie-team · Oct 2, 2024 · d267c11 · d267c11
1 parent 42ca4e2
commit d267c11
Show file tree

Hide file tree

Showing 12 changed files with 855 additions and 891 deletions.
diff --git a/api/converter/from_pb.go b/api/converter/from_pb.go
@@ -102,25 +102,19 @@ func FromChangePack(pbPack *api.ChangePack) (*change.Pack, error) {
 		return nil, err
 	}
 
-	minSyncedVersionVector, err := FromVersionVector(pbPack.MinSyncedVersionVector)
-	if err != nil {
-		return nil, err
-	}
-
 	minSyncedTicket, err := fromTimeTicket(pbPack.MinSyncedTicket)
 	if err != nil {
 		return nil, err
 	}
 
 	pack := &change.Pack{
-		DocumentKey:            key.Key(pbPack.DocumentKey),
-		Checkpoint:             fromCheckpoint(pbPack.Checkpoint),
-		Changes:                changes,
-		Snapshot:               pbPack.Snapshot,
-		IsRemoved:              pbPack.IsRemoved,
-		VersionVector:          versionVector,
-		MinSyncedVersionVector: minSyncedVersionVector,
-		MinSyncedTicket:        minSyncedTicket,
+		DocumentKey:     key.Key(pbPack.DocumentKey),
+		Checkpoint:      fromCheckpoint(pbPack.Checkpoint),
+		Changes:         changes,
+		Snapshot:        pbPack.Snapshot,
+		IsRemoved:       pbPack.IsRemoved,
+		VersionVector:   versionVector,
+		MinSyncedTicket: minSyncedTicket,
 	}
 
 	return pack, nil

diff --git a/api/docs/yorkie/v1/resources.openapi.yaml b/api/docs/yorkie/v1/resources.openapi.yaml
@@ -262,12 +262,6 @@ components:
           description: Deprecated
           title: min_synced_ticket
           type: object
-        minSyncedVersionVector:
-          $ref: '#/components/schemas/yorkie.v1.VersionVector'
-          additionalProperties: false
-          description: ""
-          title: min_synced_version_vector
-          type: object
         snapshot:
           additionalProperties: false
           description: ""

diff --git a/api/docs/yorkie/v1/yorkie.openapi.yaml b/api/docs/yorkie/v1/yorkie.openapi.yaml
@@ -470,12 +470,6 @@ components:
           description: Deprecated
           title: min_synced_ticket
           type: object
-        minSyncedVersionVector:
-          $ref: '#/components/schemas/yorkie.v1.VersionVector'
-          additionalProperties: false
-          description: ""
-          title: min_synced_version_vector
-          type: object
         snapshot:
           additionalProperties: false
           description: ""

diff --git a/api/yorkie/v1/resources.pb.go b/api/yorkie/v1/resources.pb.go
diff --git a/api/yorkie/v1/resources.proto b/api/yorkie/v1/resources.proto
@@ -46,7 +46,6 @@ message ChangePack {
   repeated Change changes = 4;
   bool is_removed = 6;
   VersionVector version_vector = 7;
-  VersionVector min_synced_version_vector = 8;
 
   TimeTicket min_synced_ticket = 5; // Deprecated
 }

diff --git a/pkg/document/change/pack.go b/pkg/document/change/pack.go
@@ -40,11 +40,6 @@ type Pack struct {
 	// 2. In response(Snapshot), it is the version vector of the snapshot of the document.
 	VersionVector time.VersionVector
 
-	// TODO(hackerwins): Consider to merge MinSyncedVersionVector with VersionVector.
-	// MinSyncedVersionVector is the minimum version vector taken by clients who
-	// attach the document.
-	MinSyncedVersionVector time.VersionVector
-
 	// IsRemoved is a flag that indicates whether the document is removed.
 	IsRemoved bool
 

diff --git a/pkg/document/document.go b/pkg/document/document.go
@@ -184,7 +184,9 @@ func (d *Document) Update(
 // ApplyChangePack applies the given change pack into this document.
 func (d *Document) ApplyChangePack(pack *change.Pack) error {
 	// 01. Apply remote changes to both the cloneRoot and the document.
-	if len(pack.Snapshot) > 0 {
+	hasSnapshot := len(pack.Snapshot) > 0
+
+	if hasSnapshot {
 		d.cloneRoot = nil
 		d.clonePresences = nil
 		if err := d.doc.applySnapshot(pack.Snapshot, pack.Checkpoint.ServerSeq, pack.VersionVector); err != nil {
@@ -215,13 +217,13 @@ func (d *Document) ApplyChangePack(pack *change.Pack) error {
 	d.doc.checkpoint = d.doc.checkpoint.Forward(pack.Checkpoint)
 
 	// 04. Do Garbage collection.
-	if !d.options.DisableGC {
-		d.GarbageCollect(pack.MinSyncedVersionVector)
+	if !d.options.DisableGC && !hasSnapshot {
+		d.GarbageCollect(pack.VersionVector)
 	}
 
 	// 05. Remove detached client's lamport from version vector if it exists
-	if pack.MinSyncedVersionVector != nil {
-		actorIDs, err := pack.MinSyncedVersionVector.Keys()
+	if pack.VersionVector != nil && !hasSnapshot {
+		actorIDs, err := pack.VersionVector.Keys()
 		if err != nil {
 			return err
 		}

diff --git a/pkg/document/document_test.go b/pkg/document/document_test.go
@@ -557,6 +557,10 @@ func TestDocument(t *testing.T) {
 		docB := document.New("doc")
 		docB.SetActor(actorB)
 		assert.Equal(t, "{}", docB.VersionVector().Marshal())
+		// NOTE(JOOHOJANG): Normally, docB's Lamport timestamp should be included in pack.versionVector because pack is applied after docB is attached.
+		// However, since this is not the case in this test method, docB's Lamport timestamp is manually added to packA's versionVector.
+		// In actual use, since changePacks cannot be exchanged directly between clients without going through a server, the following handling was added.
+		packA.VersionVector.Set(docB.ActorID(), docB.VersionVector().VersionOf(docB.ActorID()))
 		assert.NoError(t, docB.ApplyChangePack(packA))
 		assert.Equal(t, "{000000000000000000000001:2,000000000000000000000002:3}", docB.VersionVector().Marshal())
 

diff --git a/pkg/document/internal_document.go b/pkg/document/internal_document.go
@@ -144,8 +144,10 @@ func (d *InternalDocument) HasLocalChanges() bool {
 
 // ApplyChangePack applies the given change pack into this document.
 func (d *InternalDocument) ApplyChangePack(pack *change.Pack, disableGC bool) error {
+	hasSnapshot := len(pack.Snapshot) > 0
+
 	// 01. Apply remote changes to both the cloneRoot and the document.
-	if len(pack.Snapshot) > 0 {
+	if hasSnapshot {
 		if err := d.applySnapshot(pack.Snapshot, pack.Checkpoint.ServerSeq, pack.VersionVector); err != nil {
 			return err
 		}
@@ -167,15 +169,15 @@ func (d *InternalDocument) ApplyChangePack(pack *change.Pack, disableGC bool) er
 	// 03. Update the checkpoint.
 	d.checkpoint = d.checkpoint.Forward(pack.Checkpoint)
 
-	if !disableGC && pack.MinSyncedTicket != nil {
-		if _, err := d.GarbageCollect(pack.MinSyncedVersionVector); err != nil {
+	if !disableGC && pack.VersionVector != nil && !hasSnapshot {
+		if _, err := d.GarbageCollect(pack.VersionVector); err != nil {
 			return err
 		}
 	}
 
 	// 04. Remove detached client's lamport from version vector if it exists
-	if pack.MinSyncedVersionVector != nil {
-		actorIDs, err := pack.MinSyncedVersionVector.Keys()
+	if pack.VersionVector != nil && !hasSnapshot {
+		actorIDs, err := pack.VersionVector.Keys()
 		if err != nil {
 			return err
 		}

diff --git a/server/packs/packs.go b/server/packs/packs.go
@@ -140,7 +140,9 @@ func PushPull(
 	if err != nil {
 		return nil, err
 	}
-	respPack.MinSyncedVersionVector = minSyncedVersionVector
+	if respPack.SnapshotLen() == 0 {
+		respPack.VersionVector = minSyncedVersionVector
+	}
 
 	// TODO(hackerwins): This is a previous implementation before the version
 	// vector was introduced. But it is necessary to support the previous
@@ -214,7 +216,7 @@ func PushPull(
 				ctx,
 				be,
 				docInfo,
-				minSyncedTicket,
+				minSyncedVersionVector,
 			); err != nil {
 				logging.From(ctx).Error(err)
 			}

diff --git a/server/packs/serverpacks.go b/server/packs/serverpacks.go
@@ -48,11 +48,6 @@ type ServerPack struct {
 	// 2. In response(Snapshot), it is the version vector of the snapshot of the document.
 	VersionVector time.VersionVector
 
-	// TODO(hackerwins): Consider to merge MinSyncedVersionVector with VersionVector.
-	// MinSyncedVersionVector is the minimum version vector taken by clients who
-	// attach the document.
-	MinSyncedVersionVector time.VersionVector
-
 	// IsRemoved is a flag that indicates whether the document is removed.
 	IsRemoved bool
 
@@ -150,15 +145,6 @@ func (p *ServerPack) ToPBChangePack() (*api.ChangePack, error) {
 
 	pbPack.VersionVector = pbVersionVector
 
-	if p.MinSyncedVersionVector != nil {
-		pbMinSyncedVersionVector, err := converter.ToVersionVector(p.MinSyncedVersionVector)
-		if err != nil {
-			return nil, err
-		}
-
-		pbPack.MinSyncedVersionVector = pbMinSyncedVersionVector
-	}
-
 	return pbPack, nil
 }
 

diff --git a/server/packs/snapshots.go b/server/packs/snapshots.go
@@ -31,7 +31,7 @@ func storeSnapshot(
 	ctx context.Context,
 	be *backend.Backend,
 	docInfo *database.DocInfo,
-	minSyncedTicket *time.Ticket,
+	minSyncedVersionVector time.VersionVector,
 ) error {
 	// 01. get the closest snapshot's metadata of this docInfo
 	docRefKey := docInfo.RefKey()
@@ -91,13 +91,19 @@ func storeSnapshot(
 		nil,
 		nil,
 	)
-	pack.MinSyncedTicket = minSyncedTicket
 
 	if err := doc.ApplyChangePack(pack, be.Config.SnapshotDisableGC); err != nil {
 		return err
 	}
 
-	// 04. save the snapshot of the docInfo
+	// 04. perform garbage collect to remove tombstones
+	if !be.Config.SnapshotDisableGC {
+		if _, err := doc.GarbageCollect(minSyncedVersionVector); err != nil {
+			return err
+		}
+	}
+
+	// 05. save the snapshot of the docInfo
 	if err := be.DB.CreateSnapshotInfo(
 		ctx,
 		docRefKey,
@@ -106,7 +112,7 @@ func storeSnapshot(
 		return err
 	}
 
-	// 05. delete changes before the smallest in `syncedseqs` to save storage.
+	// 06. delete changes before the smallest in `syncedseqs` to save storage.
 	if be.Config.SnapshotWithPurgingChanges {
 		if err := be.DB.PurgeStaleChanges(
 			ctx,