Preserve Detached Client's Lamport in Version Vector #931
Conversation
Since min version vector contains every client's lamport, we don't need to consider detached client's lamport has been removed from min version vector
…en someone detaches
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/sdk/src/document/document.tsOops! Something went wrong! :( ESLint: 8.19.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/sdk".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/sdk/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
packages/sdk/test/integration/gc_test.ts (1)
604-608: Reminder: Address the TODO comment regarding version vector for detached clientsThere's a TODO comment at lines 604-608 indicating the need to consider removing detached clients' lamport from the version vector. It's important to resolve or track this before merging to maintain code quality and prevent potential issues related to garbage collection and synchronization.
Would you like assistance in addressing this TODO? I can help implement the necessary changes or open a new GitHub issue to track this task.
packages/sdk/src/document/document.ts (1)
Line range hint
1251-1257: Remove unusedfilterVersionVectormethodWith the removal of its usage in
applyChangePack, thefilterVersionVectormethod defined at lines 1251-1257 appears to be unused. It's a good practice to remove such unused code to improve maintainability and reduce potential confusion.Consider removing the unused method:
- private filterVersionVector(minSyncedVersionVector: VersionVector) { - const versionVector = this.changeID.getVersionVector(); - const filteredVersionVector = versionVector.filter(minSyncedVersionVector); - - this.changeID = this.changeID.setVersionVector(filteredVersionVector); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk/src/document/document.ts(1 hunks)packages/sdk/src/document/time/version_vector.ts(1 hunks)packages/sdk/test/integration/gc_test.ts(9 hunks)
🔇 Additional comments (6)
packages/sdk/test/integration/gc_test.ts (4)
1696-1697: Duplicate TODO comment identified
The same TODO comment about considering the removal of detached clients' lamport from the version vector is present at lines 1696-1697. Ensure that you address this to maintain consistency and prevent potential synchronization issues.
Would you like assistance in resolving this TODO?
2004-2006: Duplicate TODO comment identified
A similar TODO comment is found at lines 2004-2006 regarding the consideration of removing detached clients' lamport from the version vector. Addressing this will help maintain code consistency and functionality.
Would you like assistance in addressing this TODO?
2193-2194: Duplicate TODO comment identified
The TODO comment about removing detached clients' lamport from the version vector appears again at lines 2193-2194. It's crucial to handle this to ensure accurate version vector management.
Would you like assistance in resolving this TODO?
2041-2228: New test case effectively covers garbage collection with detached clients
The newly added test case detach gc test from lines 2041-2228 thoroughly tests the garbage collection process involving detached clients. It simulates interactions among multiple clients, updates, detaches one client, and checks the integrity of version vectors and garbage collection lengths. The assertions appear correct and the test enhances coverage of edge cases related to detached clients.
packages/sdk/src/document/time/version_vector.ts (1)
99-99: Simplification of afterOrEqual logic appears correct
The adjustment in the afterOrEqual method at line 99 directly returns false when lamport is undefined, which simplifies the method and removes unnecessary dependency on the removed minLamport function. This change aligns with the intended logic where an undefined lamport indicates that the version vector does not have an entry for the actor.
packages/sdk/src/document/document.ts (1)
1236-1239: Ensure removal of version vector filtering does not introduce side effects
The filtering step for detached clients' lamport timestamps from the version vector in applyChangePack has been removed at lines 1236-1239. While this simplifies the control flow, it's important to verify that this change does not cause unintended side effects, especially in garbage collection and version synchronization.
Please run the following script to check for any remaining references to the removed filtering logic and confirm that no other parts of the codebase rely on it:
This will help ensure that the removal doesn't affect other functionalities inadvertently.
✅ Verification successful
Let me gather more context about the usage and dependencies of this method.
Let me gather one more piece of information about the change pack handling to understand the complete context.
Based on the gathered evidence, I can now provide the final response:
Removal of version vector filtering is safe and correct
The version vector filtering was redundant as garbage collection already handles the necessary version vector synchronization. The garbageCollect method is properly called with the change pack's version vector during applyChangePack, making the separate filtering step unnecessary. The removal simplifies the code without introducing side effects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of 'filterVersionVector' method in the codebase
# Find all occurrences of 'filterVersionVector'
rg 'filterVersionVector' -A 3 -B 3
Length of output: 687
Script:
#!/bin/bash
# Search for references to filterVersionVector method calls
ast-grep --pattern 'filterVersionVector($_)'
# Search for any version vector filtering related code
rg -B 3 -A 3 'filter.*VersionVector|VersionVector.*filter'
# Look for any garbage collection related code that might be affected
rg -B 3 -A 3 'garbageCollect|GarbageCollect'
Length of output: 19111
Script:
#!/bin/bash
# Search for applyChangePack implementation and its related code
ast-grep --pattern 'applyChangePack($_) {
$$$
}'
# Look for version vector usage in change pack handling
rg -B 3 -A 3 'pack.*VersionVector|VersionVector.*pack'
Length of output: 2008
hackerwins
changed the title
What this PR does / why we need it?
Since we found GC error in yorkie-team/yorkie#1089.
We decided to leave detached client's lamport in version vector for now and look for a solution later.
I modified following items
Any background context you want to provide?
What are the relevant tickets?
Fixes yorkie-team/yorkie#1089
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests