Skip to content

Add minLamport for proper GC of deactivated clients #1060

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.

Sign up for GitHub

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

Merged
hackerwins merged 2 commits into from
Nov 8, 2024
Merged

Add minLamport for proper GC of deactivated clients #1060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be4b5c3
Define MinLamport to run gc properly
JOOHOJANG Nov 7, 2024
4b3c6a4
Add TC which checks whether gc removes node made by deactivated client
JOOHOJANG Nov 7, 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
26 changes: 24 additions & 2 deletions pkg/document/time/version_vector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package time

import (
"bytes"
"math"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -114,7 +115,15 @@ func (v VersionVector) AfterOrEqual(other VersionVector) bool {

// EqualToOrAfter returns whether this VersionVector's every field is equal or after than given ticket.
func (v VersionVector) EqualToOrAfter(other *Ticket) bool {
return v[other.actorID.bytes] >= other.lamport
clientLamport, ok := v[other.actorID.bytes]

if !ok {
minLamport := v.MinLamport()

return minLamport > other.lamport
}

return clientLamport >= other.lamport
}

// Min returns new vv consists of every min value in each column.
Expand Down Expand Up @@ -167,7 +176,20 @@ func (v VersionVector) Max(other VersionVector) VersionVector {
return maxVV
}

// MaxLamport returns new vv consists of every max value in each column.
// MinLamport returns min lamport value in version vector.
func (v VersionVector) MinLamport() int64 {
var minLamport int64 = math.MaxInt64

for _, value := range v {
if value < minLamport {
minLamport = value
}
}

return minLamport
}

// MaxLamport returns max lamport value in version vector.
func (v VersionVector) MaxLamport() int64 {
var maxLamport int64 = -1

Expand Down
60 changes: 60 additions & 0 deletions test/integration/gc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,4 +1136,64 @@ func TestGarbageCollection(t *testing.T) {
assert.Equal(t, 2, d2.GarbageLen())
}
})

t.Run("gc targeting nodes made by deactivated client", func(t *testing.T) {
ctx := context.Background()
d1 := document.New(helper.TestDocKey(t))
assert.NoError(t, c1.Attach(ctx, d1))
// d2.vv =[c1:1], minvv =[c1:1], db.vv {c1: [c1:1]}
assert.Equal(t, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 1)), true)

d2 := document.New(helper.TestDocKey(t))
assert.NoError(t, c2.Attach(ctx, d2))
// d2.vv =[c1:1, c2:1], minvv =[c1:0, c2:0], db.vv {c1: [c1:1], c2: [c2:1]}
assert.Equal(t, checkVV(d2.VersionVector(), versionOf(d1.ActorID(), 1), versionOf(d2.ActorID(), 2)), true)

err := d1.Update(func(root *json.Object, p *presence.Presence) error {
root.SetNewText("text").Edit(0, 0, "a").Edit(1, 1, "b").Edit(2, 2, "c")
return nil
}, "sets text")
//d1.vv = [c1:2]
assert.Equal(t, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 2)), true)
assert.NoError(t, err)

assert.NoError(t, c1.Sync(ctx))
// d1.vv =[c1:3, c2:1], minvv =[c1:0, c2:0], db.vv {c1: [c1:2], c2: [c2:1]}
assert.Equal(t, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 3), versionOf(d2.ActorID(), 1)), true)

assert.NoError(t, c2.Sync(ctx))
// d2.vv =[c1:2, c2:3], minvv =[c1:0, c2:0], db.vv {c1: [c1:2], c2: [c1:1, c2:2]}
assert.Equal(t, checkVV(d2.VersionVector(), versionOf(d1.ActorID(), 2), versionOf(d2.ActorID(), 3)), true)

err = d2.Update(func(root *json.Object, p *presence.Presence) error {
root.GetText("text").Edit(2, 2, "c")
return nil
}, "insert c")
//d2.vv =[c1:2, c2:4]
assert.Equal(t, checkVV(d2.VersionVector(), versionOf(d1.ActorID(), 2), versionOf(d2.ActorID(), 4)), true)
assert.NoError(t, err)

err = d1.Update(func(root *json.Object, p *presence.Presence) error {
root.GetText("text").Edit(1, 3, "")
return nil
}, "delete bd")
//d1.vv = [c1:4, c2:1]
assert.Equal(t, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 4), versionOf(d2.ActorID(), 1)), true)
assert.NoError(t, err)
assert.Equal(t, 2, d1.GarbageLen())
assert.Equal(t, 0, d2.GarbageLen())

assert.NoError(t, c1.Sync(ctx))
assert.NoError(t, c2.Sync(ctx))
assert.NoError(t, c1.Deactivate(ctx))

assert.Equal(t, d2.GarbageLen(), 2)
assert.Equal(t, len(d2.VersionVector()), 2)

// TODO(JOOHOJANG): remove below comments after https://github.com/yorkie-team/yorkie/issues/1058 resolved
// Due to https://github.com/yorkie-team/yorkie/issues/1058, removing deactivated client's version vector is not working properly now.
// assert.NoError(t, c2.Sync(ctx))
// assert.Equal(t, d2.GarbageLen(), 0)
// assert.Equal(t, len(d2.VersionVector()), 1)
Comment on lines +1193 to +1197

@coderabbitai coderabbitai Bot Nov 7, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Based on the search results, I can now provide a final response:

Issue #1058 needs to be fixed to properly cleanup version vectors during client deactivation

The commented test code in test/integration/gc_test.go reveals that when a client is deactivated, its version vector is not being properly removed from the database. This is a critical issue because:

  1. The version vector of a deactivated client remains in the database, affecting the min version vector calculation
  2. This prevents proper garbage collection since the min version vector includes the deactivated client's vector
  3. The test expects d2.VersionVector() length to be 1 after deactivation, but it remains at 2

The implementation in server/backend/database/mongo/client.go shows that while client status is updated to "deactivated", there's no corresponding cleanup of the version vector.

🔗 Analysis chain

Critical issue: Version vector cleanup after client deactivation

The TODO comments reference issue #1058 which affects the proper cleanup of version vectors after client deactivation. This is a critical issue as it impacts garbage collection efficiency.

Let's verify if this issue has been addressed in other parts of the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for related version vector cleanup code
# Test: Search for version vector cleanup implementations
rg -A 5 "removeVersionVector|cleanupVersionVector|deactivateClient"

# Test: Search for related tests
rg -A 5 "TestDeactivateClient|TestVersionVectorCleanup"

Length of output: 2013

Script:

#!/bin/bash
# Let's search for the actual issue #1058 and version vector related code
# Search for the issue number in comments
rg -l "1058"

# Search for version vector related code and tests
rg -A 5 "VersionVector|versionVector"

# Search for garbage collection related tests
rg -A 5 "TestGarbageCollection|GarbageLen"

# Search for client deactivation implementation
ast-grep --pattern 'func $_ DeactivateClient($$$) {
  $$$
}'

Length of output: 201937

Script:

#!/bin/bash
# Let's search for issue #1058 and related code
rg -l "1058"

# Search for version vector cleanup in deactivate client implementation
rg -A 5 "DeactivateClient.*func"

# Search for version vector cleanup related code
rg -A 5 "removeVersionVector|cleanupVersionVector|filterVersionVector"

# Search for the actual implementation of client deactivation
ast-grep --pattern 'func $_ DeactivateClient($$$) {
  $$$
  UpdateVersionVector($$$)
  $$$
}'

Length of output: 1770

Script:

#!/bin/bash
# Let's search for the actual implementation of DeactivateClient and version vector cleanup
rg -A 10 "func.*DeactivateClient"

# Search for the issue #1058 content
rg -A 5 "1058" test/integration/gc_test.go

# Search for version vector cleanup in client deactivation
rg -A 10 "deactivate.*client.*version.*vector"

# Search for client deactivation related code
ast-grep --pattern 'func $_ Deactivate($$$) {
  $$$
}'

Length of output: 12616

})
}