Skip to content

Filter detached client from vv #1278

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

JOOHOJANG
Copy link
Contributor

@JOOHOJANG JOOHOJANG commented May 21, 2025

What this PR does / why we need it:
Since version vector's size grow infinitely, it causes performance and memory issues.

We used the min version vector to remove the Lamport of detached clients.

A separate database table was created to store detached clients. When a user detaches, their information is added to this table. Later, during a push-pull operation, we compare the computed min version vector with the list of detached clients. If minVV[client] == detached client's Lamport, we remove the corresponding key from the min version vector. This ensures that when a user receives a sync pack, their version vector is filtered accordingly.

Conceptually, removing a user's Lamport from the min version vector implies that all users have acknowledged the user's detachment. This condition can be confirmed when minVV[detached client ID] == detached client’s Lamport

Although introducing a separate table may raise concerns about potential performance overhead, the impact is expected to be minimal.

Changed

  • Add new database table (detached clients)
  • Filter min version vector using detached clients table(using cache to improve performance)
  • Filter every client's local version vector when receiving change pack
  • Add related TCs
image

Next tasks

  • Update memory db in same ways
  • Implement local version vector update logic into SDKs
  • Testing in many ways

Which issue(s) this PR fixes:

Fixes #1144

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Addressed and resolved all CodeRabbit review comments
  • Didn't break anything

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Introduced tracking and management of detached clients to improve document synchronization and garbage collection.
  • Bug Fixes
    • Enhanced version vector handling to exclude detached clients, ensuring accurate document state and cleanup.
  • Tests
    • Added integration tests to verify correct garbage collection behavior after client detachment.
  • Chores
    • Updated database schema and indexing to support detached client tracking.
    • Extended document editing and styling operations with enhanced version vector parameters for improved consistency.

Copy link

coderabbitai bot commented May 21, 2025

Walkthrough

This change introduces explicit management of detached clients' Lamport timestamps in version vectors throughout the document, database, and server layers. It adds new logic and data structures to filter, cache, and persist Lamport information for detached clients, ensuring correct garbage collection and version vector minimization. Tests are updated to verify the revised behavior.

Changes

File(s) Change Summary
pkg/document/crdt/rga_tree_split.go Extended edit and deleteNodes methods to accept minVersionVector for GC filtering.
pkg/document/document.go
pkg/document/internal_document.go
Updated ApplyChangePack and applyChanges to filter version vectors, removing detached clients' Lamport timestamps.
pkg/document/time/version_vector.go Added IsEmpty, MinLamport methods; updated EqualToOrAfter and Filter logic to handle detached clients.
server/backend/database/detached_clients.go Added new DetachedClients struct to track detached clients' Lamport and server sequence info.
server/backend/database/mongo/client.go Added detached clients LRU cache; updated version vector update and retrieval logic to filter and persist detached clients.
server/backend/database/mongo/indexes.go Added new MongoDB collection and unique index for detached clients.
test/integration/gc_test.go Added and updated tests to verify garbage collection and version vector filtering for detached clients.
pkg/document/crdt/text.go
pkg/document/crdt/tree.go
pkg/document/json/text.go
pkg/document/json/tree.go
Extended editing and styling methods to accept minVersionVector parameter and propagate it through calls.
pkg/document/change/change.go
pkg/document/operations/*.go (add, array_set, edit, increase, move, remove, set, style, tree_edit, tree_style, operation)
Updated Execute method signatures to accept additional minVersionVector parameter; propagated through operations.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Server
    participant DB
    participant DetachedClientsColl as DetachedClients Collection

    Client->>Server: Detach from document
    Server->>DB: Remove client from version vector
    Server->>DetachedClientsColl: Upsert {clientID, lamport}
    Server->>Client: Acknowledge detachment

    Note over Server,DB: On min version vector calculation
    Server->>DB: Load version vectors for all clients
    Server->>DetachedClientsColl: Load detached clients' lamports
    Server->>Server: Filter out detached clients from minVV if lamport matches
    Server->>Client: Send filtered minVV
Loading
sequenceDiagram
    participant Document
    participant ChangePack
    participant VersionVector

    Document->>ChangePack: Receive pack with VersionVector
    Document->>VersionVector: Filter to remove detached clients
    Document->>Document: Update local VersionVector
Loading

Assessment against linked issues

Objective Addressed Explanation
Remove detached client's Lamport from version vectors, filter only when all users have received detach (#1144)
Store detached client data separately and remove from minVV only when minVV[detachedClientID] matches
Remove from local document VV when receiving presence clear change
Maintain correct GC and concurrency handling despite VV minimization

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes detected.

Possibly related PRs

  • yorkie-team/yorkie#1090: Also addresses handling of detached clients' Lamport timestamps in version vectors and their effect on garbage collection.
  • yorkie-team/yorkie#1096: Modifies similar methods (deleteNodes, ApplyChangePack) to improve version vector and Lamport timestamp logic.
  • yorkie-team/yorkie#1060: Adds MinLamport and updates version vector logic, which is extended by this PR for detached client handling.

Suggested reviewers

  • hackerwins
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@hackerwins hackerwins force-pushed the main branch 3 times, most recently from 5aa1cf8 to 16e3c4a Compare May 26, 2025 02:05
@raararaara raararaara changed the title Filter detahced client from vv Filter detached client from vv May 26, 2025
@hackerwins hackerwins force-pushed the main branch 3 times, most recently from ee9bbd2 to a98ac24 Compare May 27, 2025 08:37
@JOOHOJANG JOOHOJANG marked this pull request as ready for review May 29, 2025 18:05
@JOOHOJANG JOOHOJANG requested review from hackerwins and chacha912 May 29, 2025 18:05
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (2)
test/integration/gc_test.go (1)

1191-1191: ⚠️ Potential issue

Duplicate test names detected.

There are two tests named "detach gc test" in the same test suite (lines 1191 and 1529). This will cause confusion and may lead to test framework issues. Please rename one of them to have a unique, descriptive name.

Consider renaming them to be more specific about what each test validates, for example:

  • "detach gc with multiple clients test"
  • "detach gc with complex text operations test"

Also applies to: 1529-1529

server/backend/database/mongo/client.go (1)

1349-1491: 🛠️ Refactor suggestion

Consider refactoring this method to reduce complexity.

The UpdateAndFindMinVersionVector method has grown significantly in complexity with the addition of detached client handling. It now has multiple responsibilities:

  1. Updating version vectors
  2. Managing cache state
  3. Calculating minimum version vector
  4. Filtering detached clients
  5. Cleaning up stale detached client data

This makes the method difficult to understand and maintain.

Consider breaking this into smaller, focused methods:

  • updateClientVersionVector()
  • loadVersionVectorsIntoCache()
  • filterDetachedClients()
  • cleanupStaleDetachedClients()
🧰 Tools
🪛 GitHub Check: build

[failure] 1456-1456:
undefined: clientInfo


[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Check: load-test

[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Check: complex-test

[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Actions: CI

[error] 1456-1456: Undefined identifier 'clientInfo' at line 1456.

🧹 Nitpick comments (2)
server/backend/database/detached_clients.go (1)

21-21: Fix line length violation.

The comment on line 21 exceeds the maximum line length of 120 characters. Please break it into multiple lines.

-// DetachedClients is a structure representing information about the detached client's lamport for each document and client.
+// DetachedClients is a structure representing information about the detached
+// client's lamport for each document and client.
🧰 Tools
🪛 golangci-lint (1.64.8)

21-21: The line is 124 characters long, which exceeds the maximum of 120 characters.

(lll)

pkg/document/time/version_vector.go (1)

164-174: Consider adding documentation for the modified behavior.

The EqualToOrAfter method now has different behavior when the actor ID is not present in the version vector. This change in semantics should be documented in the method's comment to clarify when and why it returns true for missing actors.

-// EqualToOrAfter returns whether this VersionVector's every field is equal or after than given ticket.
+// EqualToOrAfter returns whether this VersionVector's every field is equal or after than given ticket.
+// If the ticket's actor is not present in this version vector, it returns true if the minimum
+// lamport value in this vector is greater than the ticket's lamport, indicating the ticket
+// is from a detached client that has been garbage collected.
 func (v VersionVector) EqualToOrAfter(other *Ticket) bool {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df39258 and 98fa339.

📒 Files selected for processing (8)
  • pkg/document/crdt/rga_tree_split.go (1 hunks)
  • pkg/document/document.go (1 hunks)
  • pkg/document/internal_document.go (1 hunks)
  • pkg/document/time/version_vector.go (3 hunks)
  • server/backend/database/detached_clients.go (1 hunks)
  • server/backend/database/mongo/client.go (7 hunks)
  • server/backend/database/mongo/indexes.go (2 hunks)
  • test/integration/gc_test.go (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/document/internal_document.go (2)
pkg/document/time/version_vector.go (1)
  • VersionVector (32-32)
api/yorkie/v1/resources.pb.go (3)
  • VersionVector (531-537)
  • VersionVector (552-552)
  • VersionVector (567-569)
pkg/document/crdt/rga_tree_split.go (1)
pkg/document/time/ticket.go (1)
  • MaxLamport (31-31)
pkg/document/time/version_vector.go (1)
api/yorkie/v1/resources.pb.go (3)
  • VersionVector (531-537)
  • VersionVector (552-552)
  • VersionVector (567-569)
🪛 golangci-lint (1.64.8)
server/backend/database/detached_clients.go

21-21: The line is 124 characters long, which exceeds the maximum of 120 characters.

(lll)

🪛 GitHub Check: build
server/backend/database/mongo/client.go

[failure] 1456-1456:
undefined: clientInfo


[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Check: load-test
server/backend/database/mongo/client.go

[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Check: complex-test
server/backend/database/mongo/client.go

[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Actions: CI
server/backend/database/mongo/client.go

[error] 1456-1456: Undefined identifier 'clientInfo' at line 1456.

🔇 Additional comments (8)
server/backend/database/mongo/indexes.go (2)

43-44: LGTM! New collection constant follows naming conventions.

The constant definition is consistent with other collection constants in the file.


155-165: LGTM! Index configuration follows established patterns.

The index definition for the new detached clients collection correctly:

  • Uses doc_id as the shard key for horizontal scaling
  • Creates a unique compound index on (doc_id, project_id, client_id) to ensure uniqueness per document/project/client combination
  • Follows the same pattern used by other collections like ColVersionVectors

This will enable efficient storage and querying of detached clients' lamport information.

server/backend/database/detached_clients.go (1)

22-29: LGTM! Well-structured data model for detached clients.

The DetachedClients struct appropriately models the required information:

  • Unique identifiers for project, document, and client
  • Lamport timestamp for version tracking
  • Server sequence for ordering
  • Proper BSON tags for MongoDB serialization

This design supports the efficient storage and retrieval of detached client information needed for version vector filtering.

pkg/document/crdt/rga_tree_split.go (1)

553-559: LGTM! Improved handling of missing actor IDs in version vector.

The change from 0 to vector.MaxLamport() for missing actor IDs is correct. When an actor ID is not found in the version vector:

  • Previous logic (0): Assumed the client hasn't seen any operations from that actor
  • New logic (vector.MaxLamport()): Assumes the client has seen all operations from that actor

This change aligns with the detached client filtering feature where missing actors (likely detached clients) should be treated as "fully acknowledged" rather than "never seen", ensuring consistent garbage collection behavior.

pkg/document/internal_document.go (1)

195-205:

✅ Verification successful

LGTM! Correct implementation of detached client filtering.

The new step effectively filters out detached clients' lamport entries from the document's version vector. The logic is sound:

  1. Correct conditions: Only applies when pack.VersionVector is present and no snapshot was applied
  2. Proper filtering: Extracts actor IDs from the pack and filters the document's version vector to retain only those actors
  3. Error handling: Appropriately returns errors from the Keys() method
  4. Performance consideration: Only performs filtering when the current version vector is not empty

This ensures that detached clients are properly removed from version vectors during change pack application, supporting the PR's objective of preventing infinite version vector growth.

Let me verify that the Filter method works as expected for this use case:


🏁 Script executed:

#!/bin/bash
# Description: Verify the Filter method implementation and usage pattern
# Expected: Filter method should retain only actor IDs present in the provided slice

# Search for Filter method implementation in version vector
ast-grep --pattern 'func ($_ VersionVector) Filter($_) $_'

# Search for Filter method usage patterns
rg -A 3 -B 3 'Filter\(' --type go

Length of output: 4606


Detached client filtering logic confirmed

Verified that VersionVector.Filter (pkg/document/time/version_vector.go:243–255) correctly retains only those actor IDs present in the original vector. No further changes needed—filtering semantics are sound, error handling is appropriate, and performance considerations are covered.

LGTM! Approving these changes.

pkg/document/document.go (1)

225-236: LGTM! Clean implementation of detached client filtering.

The logic correctly filters out detached clients' Lamport timestamps from the document's version vector after applying changes. The conditional checks ensure this only happens when appropriate (when there's a version vector in the pack and no snapshot).

server/backend/database/mongo/client.go (2)

1442-1456: Investigate undefined variable error reported by static analysis.

Static analysis tools report an undefined clientInfo variable at line 1456, but the code appears to use clientInfo correctly as a function parameter. This might indicate a build configuration issue or a false positive.

Please verify that the code compiles correctly in your local environment. If it does, this might be a CI/CD configuration issue that needs investigation.

🧰 Tools
🪛 GitHub Check: build

[failure] 1456-1456:
undefined: clientInfo


[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Check: load-test

[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Check: complex-test

[failure] 1456-1456:
undefined: clientInfo

🪛 GitHub Actions: CI

[error] 1456-1456: Undefined identifier 'clientInfo' at line 1456.


1381-1386: Potential race condition in cache operations.

When a client detaches, the code deletes from vvCache and adds to detachedClientsCache. If another goroutine accesses these caches between these operations, it might see an inconsistent state.

Are these cache operations protected by proper synchronization at a higher level? The cmap package provides thread-safe maps individually, but the transition between caches might need additional synchronization.

Comment on lines +176 to +187
// 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
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle empty version vector case in MinLamport.

The MinLamport method will return math.MaxInt64 for an empty version vector, which might be unexpected behavior for callers.

 // MinLamport returns min lamport value in version vector.
 func (v VersionVector) MinLamport() int64 {
+	if v.IsEmpty() {
+		return 0
+	}
+
 	var minLamport int64 = math.MaxInt64
 
 	for _, value := range v {
 		if value < minLamport {
 			minLamport = value
 		}
 	}
 
 	return minLamport
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 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
}
// MinLamport returns min lamport value in version vector.
func (v VersionVector) MinLamport() int64 {
if v.IsEmpty() {
return 0
}
var minLamport int64 = math.MaxInt64
for _, value := range v {
if value < minLamport {
minLamport = value
}
}
return minLamport
}
🤖 Prompt for AI Agents
In pkg/document/time/version_vector.go around lines 176 to 187, the MinLamport
method returns math.MaxInt64 when the version vector is empty, which can be
misleading. Modify the method to check if the version vector is empty at the
start and return a more appropriate value, such as zero or an error indicator,
to clearly signal the empty case to callers.

Comment on lines +1449 to +1453
actorID, err := keys[i].ToActorID()

if actorID == currentActorID {
continue
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Missing error handling after ToActorID call.

The error from keys[i].ToActorID() is not checked before being used in the condition. This could lead to using an invalid actor ID.

 actorID, err := keys[i].ToActorID()
+if err != nil {
+    return nil, err
+}
 
 if actorID == currentActorID {
     continue
 }
-
-if err != nil {
-    return nil, err
-}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In server/backend/database/mongo/client.go around lines 1449 to 1453, the error
returned by keys[i].ToActorID() is not checked before using actorID in the
condition. Add error handling immediately after the ToActorID call to check if
err is not nil, and handle the error appropriately (e.g., return it or log it)
before proceeding to use actorID.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
pkg/document/crdt/text.go (1)

346-354: Consider using or marking the minVersionVector parameter.

The minVersionVector parameter is added to the Style method signature but doesn't appear to be used in the implementation. If this parameter is intended for future use, consider marking it as unused with _ to avoid potential linting warnings.

 func (t *Text) Style(
 	from,
 	to *RGATreeSplitNodePos,
 	attributes map[string]string,
 	executedAt *time.Ticket,
 	versionVector time.VersionVector,
-	minVersionVector time.VersionVector,
+	_ time.VersionVector, // minVersionVector - reserved for future use
 ) ([]GCPair, error) {
pkg/document/crdt/tree.go (1)

895-895: Consider using or marking the minVersionVector parameter in collectBetween.

The minVersionVector parameter is added to the collectBetween method signature but doesn't appear to be used in the implementation. If this is intended for future use, consider marking it as unused to avoid linting warnings.

 func (t *Tree) collectBetween(
 	fromParent *TreeNode, fromLeft *TreeNode,
 	toParent *TreeNode, toLeft *TreeNode,
 	editedAt *time.Ticket,
 	versionVector time.VersionVector,
-	minVersionVector time.VersionVector,
+	_ time.VersionVector, // minVersionVector - reserved for future use
 ) ([]*TreeNode, []*TreeNode, error) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d62c19 and bcf6339.

📒 Files selected for processing (20)
  • admin/client.go (1 hunks)
  • pkg/document/change/change.go (1 hunks)
  • pkg/document/crdt/rga_tree_split.go (3 hunks)
  • pkg/document/crdt/text.go (3 hunks)
  • pkg/document/crdt/tree.go (7 hunks)
  • pkg/document/document.go (4 hunks)
  • pkg/document/internal_document.go (4 hunks)
  • pkg/document/json/text.go (2 hunks)
  • pkg/document/json/tree.go (3 hunks)
  • pkg/document/operations/add.go (1 hunks)
  • pkg/document/operations/array_set.go (1 hunks)
  • pkg/document/operations/edit.go (1 hunks)
  • pkg/document/operations/increase.go (1 hunks)
  • pkg/document/operations/move.go (1 hunks)
  • pkg/document/operations/operation.go (1 hunks)
  • pkg/document/operations/remove.go (1 hunks)
  • pkg/document/operations/set.go (1 hunks)
  • pkg/document/operations/style.go (1 hunks)
  • pkg/document/operations/tree_edit.go (2 hunks)
  • pkg/document/operations/tree_style.go (2 hunks)
✅ Files skipped from review due to trivial changes (7)
  • pkg/document/json/text.go
  • pkg/document/operations/add.go
  • pkg/document/operations/array_set.go
  • pkg/document/json/tree.go
  • pkg/document/operations/increase.go
  • pkg/document/operations/move.go
  • pkg/document/operations/set.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • pkg/document/internal_document.go
  • pkg/document/document.go
  • pkg/document/crdt/rga_tree_split.go
🧰 Additional context used
🧬 Code Graph Analysis (6)
pkg/document/operations/style.go (5)
pkg/document/crdt/root.go (1)
  • Root (39-44)
pkg/document/time/version_vector.go (1)
  • VersionVector (32-32)
api/yorkie/v1/resources.pb.go (3)
  • VersionVector (531-537)
  • VersionVector (552-552)
  • VersionVector (567-569)
pkg/document/crdt/text.go (1)
  • Text (160-165)
pkg/document/operations/operation.go (1)
  • ErrNotApplicableDataType (31-31)
pkg/document/change/change.go (4)
pkg/document/crdt/root.go (1)
  • Root (39-44)
pkg/document/innerpresence/presence.go (1)
  • Map (27-29)
pkg/document/time/version_vector.go (1)
  • VersionVector (32-32)
pkg/document/change/id.go (1)
  • ID (26-48)
pkg/document/operations/edit.go (2)
pkg/document/time/version_vector.go (1)
  • VersionVector (32-32)
pkg/document/crdt/text.go (1)
  • Text (160-165)
pkg/document/operations/operation.go (2)
pkg/document/crdt/root.go (1)
  • Root (39-44)
pkg/document/time/version_vector.go (1)
  • VersionVector (32-32)
pkg/document/operations/tree_edit.go (3)
pkg/document/crdt/root.go (1)
  • Root (39-44)
pkg/document/time/version_vector.go (1)
  • VersionVector (32-32)
api/yorkie/v1/resources.pb.go (3)
  • VersionVector (531-537)
  • VersionVector (552-552)
  • VersionVector (567-569)
pkg/document/crdt/text.go (2)
pkg/document/time/version_vector.go (1)
  • VersionVector (32-32)
api/yorkie/v1/resources.pb.go (3)
  • VersionVector (531-537)
  • VersionVector (552-552)
  • VersionVector (567-569)
🔇 Additional comments (16)
admin/client.go (1)

405-405: LGTM: Correct adaptation to updated ApplyChanges signature.

The addition of nil as the first parameter to ApplyChanges correctly adapts to the updated method signature that now accepts a version vector parameter. Using nil is appropriate in this admin context where filtering detached clients' Lamport timestamps is not necessary for generating change summaries.

pkg/document/operations/tree_edit.go (1)

66-66: LGTM: Correct implementation of version vector enhancement.

The Execute method signature correctly adds the minVersionVector parameter and properly passes it through to the underlying crdt.Tree.Edit method. This change aligns with the system-wide enhancement for filtering detached clients' Lamport timestamps from version vectors.

Also applies to: 114-114

pkg/document/operations/style.go (1)

60-60: LGTM: Consistent implementation of version vector parameter.

The Execute method signature correctly adds the minVersionVector parameter and properly forwards it to the crdt.Text.Style method. The implementation is consistent with the pattern established across other operation types for enhanced version vector handling.

Also applies to: 67-67

pkg/document/change/change.go (1)

54-54: LGTM: Correct coordination of version vector parameters.

The Execute method signature correctly adds the vector parameter and properly passes both the change's version vector (c.ID().versionVector) and the minimum version vector (vector) to each operation's Execute method. This ensures consistent version vector handling throughout the change application process.

Also applies to: 56-56

pkg/document/operations/edit.go (1)

67-67: LGTM: Proper implementation of version vector enhancement.

The Execute method signature correctly adds the minVersionVector parameter and properly forwards it to the crdt.Text.Edit method. This change maintains consistency with other operation types and supports the enhanced version vector handling for detached client filtering.

Also applies to: 72-72

pkg/document/operations/operation.go (1)

37-37: Approve interface enhancement for version vector filtering.

The addition of minVersionVector parameter to the Execute method interface is consistent with the PR objective to filter detached clients from version vectors. The parameter type and naming follow existing conventions.

pkg/document/operations/remove.go (1)

51-51: Correct implementation of the updated interface.

The method signature properly implements the new Operation interface. Marking both version vector parameters as unused is appropriate since Remove operations don't require version vector filtering logic.

pkg/document/operations/tree_style.go (2)

82-82: Proper parameter propagation to CRDT methods.

The method signature is correctly updated to accept the minVersionVector parameter.


92-98: Consistent parameter passing to underlying CRDT operations.

Both Style and RemoveStyle calls correctly pass through the minVersionVector parameter, ensuring version vector filtering works properly for tree styling operations.

pkg/document/crdt/text.go (2)

322-330: Proper parameter addition to Edit method.

The minVersionVector parameter is correctly added to the method signature and will be passed through to the underlying rgaTreeSplit.edit method.


336-343:

✅ Verification successful

Verify parameter usage in rgaTreeSplit.edit.

The minVersionVector parameter is properly passed to the underlying rgaTreeSplit.edit method. Ensure that the RGA tree split implementation can handle this parameter correctly.


🏁 Script executed:

#!/bin/bash
# Description: Verify that rgaTreeSplit.edit method accepts minVersionVector parameter
# Expected: Find the edit method signature that includes minVersionVector parameter

ast-grep --pattern $'func ($_ *RGATreeSplit[$_]) edit(
  $$$,
  minVersionVector $_,
  $$$
) $$$'

Length of output: 98


🏁 Script executed:

#!/bin/bash
# Locate the rgaTreeSplit type definition
rg -n "type rgaTreeSplit" -A 3 .

# Search for the edit method on rgaTreeSplit
rg -n "func \(.*rgaTreeSplit.*\) edit" -A 5 .

# Find any occurrences of minVersionVector in the repository
rg -n "minVersionVector" -A 5 .

Length of output: 16971


Confirmed: rgaTreeSplit.edit signature includes minVersionVector

The edit method on RGATreeSplit is defined to accept minVersionVector time.VersionVector, so the parameter is correctly propagated through to the tree-split implementation. No further action required.

pkg/document/crdt/tree.go (5)

804-804: Consistent parameter addition to Edit method.

The minVersionVector parameter is properly added to the Edit method signature and will be passed through to the collectBetween method.


818-818: Parameter propagation to collectBetween method.

The minVersionVector parameter is correctly passed to the collectBetween method, maintaining the parameter flow through the editing pipeline.


1024-1024: Verify consistent nil parameter usage pattern.

Similar to EditT, this method passes nil for minVersionVector. This suggests a pattern where test/utility methods don't require version vector filtering.


1033-1034: Proper parameter addition to Style and RemoveStyle methods.

Both methods correctly include the minVersionVector parameter in their signatures, maintaining consistency with the broader version vector filtering implementation.


756-756:

✅ Verification successful

Verify nil parameter handling in underlying methods.

The minVersionVector parameter is passed as nil in this test method. Ensure that the underlying Edit method can handle nil values gracefully.


🏁 Script executed:

#!/bin/bash
# Description: Check if Edit method handles nil minVersionVector parameter safely
# Expected: Find nil checks or safe handling of minVersionVector in Edit method

rg -A 10 -B 5 "minVersionVector.*nil|nil.*minVersionVector" --type go

Length of output: 2292


🏁 Script executed:

#!/bin/bash
# Locate the Edit method in pkg/document/crdt/tree.go and inspect how it uses minVersionVector
echo "==> Edit method signature and context"
rg -n -C 3 "func .*Edit\(" pkg/document/crdt/tree.go

echo
echo "==> Usage of minVersionVector inside Edit"
rg -n -C 5 "minVersionVector" pkg/document/crdt/tree.go

Length of output: 2302


Nil minVersionVector is safe; no handling changes needed

Search through pkg/document/crdt/tree.go shows that:

  • minVersionVector is accepted by Tree.Edit (lines 797–805) and forwarded to collectBetween (lines 890–896)
  • Neither Edit nor collectBetween reference or mutate minVersionVector in their bodies

Since the parameter is currently unused, passing nil will not cause panics. You may optionally add a comment or rename it to _minVersionVector to signal it’s reserved for future use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Detached Client's Lamport from Version Vectors
2 participants