Add minLamport for proper GC of deactivated clients #1060
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client1
participant Client2
participant Document
participant GarbageCollector
Client1->>Document: Update Document
Document->>Client2: Sync Changes
Client1->>Client1: Deactivate
GarbageCollector->>Document: Check Garbage Length
GarbageCollector->>Document: Collect Garbage
GarbageCollector->>Client2: Verify Garbage Length
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
pkg/document/time/version_vector.go (2)
179-190: Consider handling empty version vectorsThe implementation is correct, but it might be worth handling the edge case of empty version vectors explicitly.
Consider this minor enhancement:
func (v VersionVector) MinLamport() int64 { + if len(v) == 0 { + return 0 // or another appropriate default value + } + var minLamport int64 = math.MaxInt64 for _, value := range v { if value < minLamport { minLamport = value } } return minLamport }
Line range hint
118-190: Architectural consideration for version vector GCThe implementation aligns well with distributed systems principles by properly handling version vectors for garbage collection. The approach of using minimum Lamport clock for comparison when actors are missing is a sound strategy for maintaining causal consistency during GC.
Consider documenting these key behaviors in the package documentation:
- How version comparison behaves when actors are missing
- The relationship between MinLamport and garbage collection
- The consistency guarantees provided by these changes
test/integration/gc_test.go (1)
1140-1198: Enhance test case documentation and assertionsThe test case structure is good, but could benefit from:
- Adding comments explaining the expected behavior after client deactivation
- Documenting the significance of version vector changes
- Adding assertions to verify the document state after garbage collection
t.Run("gc targeting nodes made by deactivated client", func(t *testing.T) { + // This test verifies that: + // 1. Garbage collection works properly when a client is deactivated + // 2. Version vectors are properly updated after client deactivation + // 3. Garbage nodes created by the deactivated client are collected ctx := context.Background() d1 := document.New(helper.TestDocKey(t)) // ... rest of the test assert.NoError(t, c1.Deactivate(ctx)) assert.Equal(t, d2.GarbageLen(), 2) assert.Equal(t, len(d2.VersionVector()), 2) + // Verify document state after deactivation + assert.Equal(t, `{"text":[{"val":"a"}]}`, d2.Marshal())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/document/time/version_vector.go(3 hunks)test/integration/gc_test.go(1 hunks)
🔇 Additional comments (2)
pkg/document/time/version_vector.go (2)
21-21: LGTM: Required import for math.MaxInt64
The math package import is correctly placed and necessary for the new MinLamport implementation.
118-126: LGTM: Enhanced version comparison logic
The updated logic properly handles missing actor versions by comparing against the minimum Lamport clock, which is essential for garbage collection when clients are deactivated.
Let's verify the usage of this method in GC-related code:
✅ Verification successful
EqualToOrAfter method changes are correctly implemented and used
The verification shows that the EqualToOrAfter method with MinLamport is properly used in garbage collection across the codebase:
- It's primarily used in
Root.GarbageCollectto determine if elements can be purged - The method is extensively tested in both unit and integration tests
- Tests verify the behavior with missing actor versions
- The implementation is used consistently in garbage collection across different CRDT types (arrays, objects, text)
The changes maintain causal consistency while enabling proper garbage collection of elements from deactivated clients.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the EqualToOrAfter method usage in GC context
# Expected: Find calls to EqualToOrAfter in GC-related code
# Search for EqualToOrAfter usage in GC context
rg -A 5 "EqualToOrAfter.*Ticket"
# Search for GC-related files that might use this method
rg -l "gc|garbage.?collect"
Length of output: 981
Script:
#!/bin/bash
# Let's check the actual usage of EqualToOrAfter in GC-related files
rg -B 3 -A 3 "EqualToOrAfter" pkg/document/crdt/gc.go pkg/document/document.go
# Also check test files to understand the expected behavior
rg -B 3 -A 3 "EqualToOrAfter" pkg/document/gc_test.go test/integration/gc_test.go
# Let's also look at the MinLamport usage since it's crucial for the new logic
rg -B 3 -A 3 "MinLamport" pkg/document/time/version_vector.go pkg/document/crdt/gc.go
Length of output: 1102
Script:
#!/bin/bash
# Let's look for any tests specifically verifying this behavior
ast-grep --pattern 'func Test$_($_) {
$$$
EqualToOrAfter
$$$
}'
# Also check for any direct callers of EqualToOrAfter
rg -B 5 -A 5 "\\.EqualToOrAfter\\(" --type go
# Look for any GC-related code that might be affected
rg -B 5 -A 5 "collectGarbage|GarbageCollect" --type go
Length of output: 25249
| // 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) |
There was a problem hiding this comment.
💡 Codebase verification
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:
- The version vector of a deactivated client remains in the database, affecting the min version vector calculation
- This prevents proper garbage collection since the min version vector includes the deactivated client's vector
- 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1060 +/- ##
==========================================
- Coverage 46.83% 46.77% -0.06%
==========================================
Files 84 84
Lines 12108 12123 +15
==========================================
Hits 5671 5671
- Misses 5873 5888 +15
Partials 564 564 ☔ View full report in Codecov by Sentry. |
hackerwins
changed the title
What this PR does / why we need it:
This change improves GC handling when nodes from deactivated
clients remain in the document. Previously, GC failed for such nodes because
the min version vector lacked information about deactivated clients.
The solution:
This approach ensures proper cleanup of orphaned nodes while maintaining data
consistency across the system.
Which issue(s) this PR fixes:
Related to yorkie-team/yorkie-js-sdk#926
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests