Fix duplicate changes when syncing and detaching #896
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes centralize around the introduction of the Changes
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: Response
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #896 +/- ##
==========================================
+ Coverage 50.64% 50.74% +0.09%
==========================================
Files 70 70
Lines 10466 10473 +7
==========================================
+ Hits 5301 5315 +14
+ Misses 4637 4631 -6
+ Partials 528 527 -1 ☔ View full report in Codecov by Sentry. |
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/packs/packs.go (5 hunks)
- server/rpc/yorkie_server.go (6 hunks)
- test/bench/push_pull_bench_test.go (4 hunks)
Additional context used
golangci-lint
server/packs/packs.go
[warning] 501-501: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 632-632: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 785-785: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 94-94: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 118-118: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 187-187: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 30-30: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 34-34: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 31-31: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 332-332: unused-parameter: parameter 'value' seems to be unused, consider removing or renaming it as _ (revive)
test/bench/push_pull_bench_test.go
[warning] 501-501: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 632-632: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 785-785: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 94-94: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 118-118: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 187-187: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 30-30: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 34-34: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 31-31: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 332-332: unused-parameter: parameter 'value' seems to be unused, consider removing or renaming it as _ (revive)
server/rpc/yorkie_server.go
[warning] 501-501: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 632-632: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 785-785: unused-parameter: parameter 'depth' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 94-94: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 118-118: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 187-187: unused-parameter: parameter 'parent' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 30-30: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 34-34: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 31-31: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
[warning] 332-332: unused-parameter: parameter 'value' seems to be unused, consider removing or renaming it as _ (revive)
Additional comments not posted (5)
server/packs/packs.go (3)
50-57: IntroducedPushPullOptionsstruct to encapsulateModeandStatusfor document synchronization.This struct enhances code readability and maintainability by grouping related parameters together, which is a good practice in software design.
Line range hint
71-88: RefactoredPushPullfunction to usePushPullOptions.The refactoring centralizes the handling of synchronization modes and document statuses, making the function's signature cleaner and its behavior easier to understand and maintain.
Also applies to: 96-112, 130-158
98-109: Enhanced error handling for document state updates.The addition of error checks after updating document status (removed, detached, checkpoint update) ensures the function's robustness by handling potential failures gracefully.
test/bench/push_pull_bench_test.go (1)
147-150: Updated benchmark tests to usePushPullOptionsstruct.Consistent use of the new struct across all benchmark functions ensures that the tests are aligned with the latest changes in the main codebase, maintaining the relevance and accuracy of the benchmarks.
Also applies to: 177-180, 187-190, 220-223, 259-262, 269-272
server/rpc/yorkie_server.go (1)
164-167: Updated server functions to usePushPullOptionsstruct.The changes in
PushPull,DetachDocument,PushPullChanges, andRemoveDocumentfunctions to use the new struct are correctly implemented. This ensures that the server's document handling logic is consistent with the updated synchronization logic.Also applies to: 258-261, 349-352, 541-544
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.
14c9b47#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.