Improve Version Vector Handling for Legacy SDK and Snapshots #933
Conversation
WalkthroughThe pull request introduces several modifications across multiple files related to the handling of logical clocks and version vectors. Key changes include updates to the Changes
Possibly related PRs
Suggested reviewers
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/change/change_id.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. 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 (4)
packages/sdk/test/integration/gc_test.ts (1)
1708-1708: Consider standardizing the docKey format across all testsThe docKey format has been changed from
`${task.name}-${new Date().getTime()}`to`${new Date().getTime()}-${task.name}`. While this change works, it's inconsistent with other tests in the file.Apply this change to maintain consistency:
-const docKey = toDocKey(`${new Date().getTime()}-${task.name}`); +const docKey = toDocKey(`${task.name}-${new Date().getTime()}`);Also applies to: 1882-1882
packages/sdk/src/document/time/version_vector.ts (1)
39-44: LGTM! Consider enhancing the documentation.The implementation is correct and follows the single responsibility principle. Consider adding more details to the documentation about when this method should be used and its implications.
/** - * `unset` removes the version for the given actor from the VersionVector. + * `unset` removes the version for the given actor from the VersionVector. + * This is typically used when cleaning up version vectors, such as removing + * initial actor IDs or handling actor disconnections. */packages/sdk/src/document/change/change_id.ts (1)
97-99: LGTM! Add comment for legacy SDK compatibility.The implementation correctly handles empty version vectors from legacy SDK versions.
+ // Handle empty version vectors from legacy SDK (v0.5.2 or below) + // by including the actor's lamport in the version vector if (other.versionVector.size() === 0) { newID.versionVector.set(other.actor, other.lamport); }packages/sdk/src/document/document.ts (1)
1453-1456: LGTM! Document the rationale for using maxLamport.The change correctly uses the maximum lamport value from the snapshot vector instead of serverSeq, which fixes character deletion issues when certain lamport values exceed serverSeq.
+ // Use maxLamport from snapshotVector instead of serverSeq to handle cases + // where some lamport values are greater than serverSeq, which can cause + // character deletion issues when setting clocks this.changeID = this.changeID.setClocks( snapshotVector.maxLamport(), snapshotVector, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/sdk/src/document/change/change_id.ts(2 hunks)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(4 hunks)
🔇 Additional comments (2)
packages/sdk/test/integration/gc_test.ts (1)
2234-2357: Well-structured test case for version vector behavior with snapshots
The test case effectively validates the version vector behavior when dealing with snapshots. It follows a clear three-step structure:
- Updates exceeding snapshot threshold
- Local changes with snapshot pull
- Deletions after snapshot
A few observations:
- The test includes proper cleanup with client deactivation
- The timeout is appropriately set to 50000ms for this long-running test
- The assertions verify both version vectors and document content length
Let's verify the test coverage and similar test patterns:
✅ Verification successful
The shell script results show comprehensive test coverage for version vector behavior in the codebase:
- The test under review is part of a larger suite of GC (garbage collection) tests in
gc_test.ts - The version vector assertions are thoroughly tested across multiple scenarios with different client combinations
- The
DefaultSnapshotThresholdconstant is used consistently across multiple test files including:- integration/gc_test.ts
- integration/snapshot_test.ts
- integration/tree_test.ts
- integration/client_test.ts
- integration/presence_test.ts
- unit/document/document_test.ts
The test case under review follows the established patterns and properly validates version vector behavior with snapshots. The test structure and assertions are consistent with other tests in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other snapshot-related tests and version vector assertions
# Look for other snapshot threshold tests
rg -A 2 "DefaultSnapshotThreshold" --type ts
# Look for version vector assertion patterns
ast-grep --pattern 'assert.equal(
versionVectorHelper($_, $_),
$_
)'
Length of output: 65754
packages/sdk/src/document/change/change_id.ts (1)
109-109: LGTM! Improved clock synchronization logic.
The changes enhance clock synchronization by:
- Ensuring proper lamport clock increment
- Cleaning up the version vector by removing the initial actor ID
Also applies to: 112-112
hackerwins
changed the title
What this PR does / why we need it?
Add
{actorID:lamport}to accumulate VV when VV is empty in applyChangeUse maxLamport() instead of serverSeq in applySnapshot
Any background context you want to provide?
What are the relevant tickets?
Related yorkie-team/yorkie#1096
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests