coderabbitai[bot]
commented
1 month ago [!IMPORTANT]
Review skipped
Draft detected.
Please check the settings in the CodeRabbit UI or the
.coderabbit.yamlfile in this repository. To trigger a single review, invoke the@coderabbitai reviewcommand.You can disable this status message by setting the
reviews.review_statustofalsein the CodeRabbit configuration file.
Walkthrough
The recent changes centralize around the introduction of the PushPullOptions struct to the PushPull function and adjustments to document handling during the detach process. This facilitates more robust management of document status and synchronizes changes, especially addressing issues with simultaneous sync and detach calls causing duplicate pushes.
Changes
| Files/Modules | Change Summary |
|---|---|
server/packs/packs.go |
Added PushPullOptions, adjusted PushPull logic, and enhanced document handling during sync and detach. |
server/rpc/yorkie_server.go |
Modified PushPull and DetachDocument functions to utilize PushPullOptions, improving status management. |
test/bench/push_pull_bench_test.go |
Updated benchmarking functions to use PushPullOptions struct for PushPull calls. |
Sequence Diagram(s)
sequenceDiagram
participant Client
participant yorkieServer
participant packs
participant Database
Client->>+yorkieServer: PushPull(pack, {Mode, Status})
yorkieServer->>+packs: PushPull(ctx, backend, project, clientInfo, docInfo, pack, {Mode, Status})
packs->>+Database: Store pushed changes, update docs and checkpoints
packs-->+yorkieServer: Response
yorkieServer-->>Client: Response
Client->>+yorkieServer: DetachDocument
yorkieServer->>+packs: PushPull(ctx, backend, project, clientInfo, docInfo, pack, {Mode, Status:Removed})
packs->>+Database: Update document status
packs-->+yorkieServer: Response
yorkieServer-->>Client: ResponseAssessment against linked issues
| Objective (Issue #) | Addressed | Explanation |
|---|---|---|
| Prevent duplicate change pushes ( #895 ) | ✅ | |
| Ensure status is correctly managed during PushPull and DetachDocument ( #895 ) | ✅ | |
| Correct client and document status handling during simultaneous sync and detach calls ( #895 ) | ✅ |
Poem
In the land where code does thrive,
Yorkie’s changes come alive.
PushPull options clear the way,
So duplicate bugs no longer stay. ✨
Sync and detach with grace,
Our docs now stay in place.📄
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?
codecov[bot]
hackerwins
What this PR does / why we need it:
Fix duplicate changes when syncing and detaching
This commit addresses the issue of duplicate changes being inserted when PushPull and Detach occur simultaneously. Previously, there was logic to filter out duplicate changes in PushPull, but during Detach, ClientInfo's Checkpoint was set to 0, preventing the filtering of duplicates.
This commit adjusts the order of updates to filter out duplicate changes before updating ClientInfo's Checkpoint, resolving the problem.
Which issue(s) this PR fixes:
Fixes #895
Special notes for your reviewer:
At first, I added a test, but race condition occurred in CI, so it was removed.
https://github.com/yorkie-team/yorkie/pull/896/commits/14c9b4700463712135e259f4ae332dfe99d46e04#diff-ef76b9ab6c8f9f1b972e664343220c8e06db4b6239ce5b22050e6d68ff66e6f5R210-R246
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
PushPullfunctionality to support additional document statuses, improving document synchronization and status handling.Bug Fixes
DetachDocumentand other related functions for consistent state updates.Performance Improvements
PushPullOptionsfor better performance insights and accuracy.