Remove deprecated MinSyncedTicket and MaxCreatedAtMapByActor #969
Conversation
WalkthroughThe changes remove legacy handling of actor-specific creation timestamps and the Changes
Possibly related PRs
Suggested reviewers
Poem
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/crdt/rga_tree_split.tsOops! Something went wrong! :( ESLint: 8.57.1 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. packages/sdk/src/document/crdt/text.tsOops! Something went wrong! :( ESLint: 8.57.1 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. packages/sdk/src/document/crdt/tree.tsOops! Something went wrong! :( ESLint: 8.57.1 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.
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/sdk/src/document/crdt/tree.ts (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #969 +/- ##
==========================================
- Coverage 78.41% 78.31% -0.10%
==========================================
Files 63 63
Lines 5602 5521 -81
Branches 1039 1013 -26
==========================================
- Hits 4393 4324 -69
+ Misses 895 892 -3
+ Partials 314 305 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR removes legacy fields and functionality related to timestamp tracking (e.g., maxCreatedAtMapByActor and minSyncedTicket) that were used during the transition from Lamport timestamps to version vectors. The changes streamline method signatures and update protobuf definitions in multiple modules.
- Legacy fields and parameters in tree and text operations have been removed.
- Protobuf resources and API converters are updated to mark deprecated fields.
- CRDT and RGA tree splitting logic is simplified to rely solely on version vectors.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/document/operation/*.ts | Removed legacy maxCreatedAtMapByActor parameter and related assignments in tree, style, edit, and tree edit operations. |
| packages/sdk/src/document/json/*.ts | Updated JSON serialization to no longer pass legacy creation maps. |
| packages/sdk/src/document/crdt/*.ts | Removed legacy comparisons based on maxCreatedAtMapByActor in favor of using only version vectors. |
| packages/sdk/src/document/change/*.ts | Cleaned up unused minSyncedTicket field and related methods. |
| packages/sdk/src/api/yorkie/v1/* | Marked deprecated fields in protobuf and updated API converters accordingly. |
| packages/sdk/src/api/converter.ts | Updated conversion logic to remove references to legacy createdAt map fields. |
Comments suppressed due to low confidence (2)
packages/sdk/src/document/crdt/tree.ts:951
- Ensure that the removal of the legacy maxCreatedAtMapByActor does not affect the correctness of deletion and styling logic by adding tests that verify behavior with version vector comparisons.
return [pairs, changes];
packages/sdk/src/api/converter.ts:396
- Verify that the removal of legacy createdAtMapByActor conversion logic does not impact client deserialization by ensuring there is adequate test coverage for protobuf serialization/deserialization.
for (const [key, value] of editOperation.getMaxCreatedAtMapByActor()) {
hackerwins
changed the title
What this PR does / why we need it?
This commit removes legacy fields and logic related to Lamport
timestamp synchronization:
MinSyncedTicketand its associated logicMaxCreatedAtMapByActorand related codeThese were previously retained for backward compatibility during the
migration to VersionVectors and are no longer needed.
Any background context you want to provide?
What are the relevant tickets?
Related yorkie-team/yorkie#1208
Checklist
Summary by CodeRabbit