Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing document detachments when client is deactivated #907

Merged
merged 23 commits into from
Jul 3, 2024

Conversation

raararaara
Copy link
Contributor

@raararaara raararaara commented Jun 27, 2024

What this PR does / why we need it?

This PR addresses the issue where documents attached to a client are not being properly detached during deactivation. Specifically, information about document detachment is not being reflected in the database, leading to situations where documents remain attached to a deactivated client.

By implementing this fix, attempting to delete a document from the dashboard will now correctly handle the scenario where the document is attached, resolving the problem outlined in issue #758.

What are the relevant tickets?

Address #758

Special notes for your reviewer:

The DB resources below should be updated when executing Client.Deactivate:

  • Update ClientInfo.Status to 'Deactivate'
  • Update ClientInfo.Documents to set ClientDocInfo.Status to 'Detached'
  • Remove entry from SyncedSeq

Updating ClientInfo involves updating a single MongoDB document, ensuring ACID compliance. However, as SyncedSeq is spread across multiple documents, ACID compliance cannot be guaranteed.

In addition, I have considered the order of updates for ClientInfo and syncedSeq, along with the potential issues or advantages that may arise.

A. ClientInfo -> SyncedSeq

  • If the ClientInfo update succeeds but the SyncedSeq update fails, the SyncedSeq may remain undeleted, leading to decreased GC efficiency. However, this issue can be addressed by adding correction logic to remove the SyncedSeq.

B. SyncedSeq -> ClientInfo

  • If the SyncedSeq is successfully deleted but the ClientInfo update fails, the tombstone might be deleted during the next GC run. This could cause sync errors when the client tries to edit again. While this scenario is unlikely, it could lead to document corruption if it occurs.

In conclusion, for this PR, we have decided to update in the order of A. ClientInfo -> SyncedSeq. We plan to add correction logic later to handle cases where the ClientInfo update succeeds but the SyncedSeq update fails.

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist

  • Added relevant tests
  • Didn't break anything

Summary by CodeRabbit

  • New Features

    • Enhanced client deactivation process to ensure documents are detached before deactivation.
  • Bug Fixes

    • Fixed issues related to document detachment when deactivating a client.
  • Tests

    • Added tests to verify the detachment of documents when a client is deactivated.

Copy link

coderabbitai bot commented Jun 27, 2024

Walkthrough

The recent updates ensure that documents are detached when a client is deactivated. This includes new error handling, added functionality in database methods, and enhanced testing scenarios to validate the changes. The Deactivate function now comprehensively handles document detachment, client info updates, and proper deactivation across various components of the server, ensuring robustness and consistency.

Changes

File(s) Change Summary
server/backend/database/client_info.go Added a new error ErrAttachedDocumentExists and a function EnsureDocumentsNotAttachedWhenDeactivated to handle document detachment during client deactivation.
server/backend/database/client_info_test.go Added test scenarios to verify that documents are detached when a client is deactivated.
server/backend/database/memory/database.go Updated DeactivateClient method to include a loop for detaching documents before deactivating the client.
server/backend/database/mongo/client.go Enhanced DeactivateClient function to also update document statuses to "detached" using a complex update operation.
server/backend/database/testcases/testcases.go Introduced functionality to ensure document detachment during client deactivation, including operations for attaching documents and updating client information.
server/clients/clients.go Enhanced Deactivate function logic to find docInfo before detaching documents, update client info after operations, and properly deactivate the client.

Poem

In code's vast land, where clients roam,
Deactivation finds its home.
Documents detached with care,
Ensuring consistency everywhere.
Robust and clean, the systems stand,
All thanks to changes, carefully planned.

Tip

AI model upgrade

gpt-4o model for reviews and chat is now live

OpenAI claims that this model is better at understanding and generating code than the previous models. Please join our Discord Community to provide any feedback or to report any issues.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go Benchmark

Benchmark suite Current: e8a90a7 Previous: ecc3ea2 Ratio
BenchmarkDocument/constructor_test 1513 ns/op 1337 B/op 24 allocs/op 1503 ns/op 1337 B/op 24 allocs/op 1.01
BenchmarkDocument/constructor_test - ns/op 1513 ns/op 1503 ns/op 1.01
BenchmarkDocument/constructor_test - B/op 1337 B/op 1337 B/op 1
BenchmarkDocument/constructor_test - allocs/op 24 allocs/op 24 allocs/op 1
BenchmarkDocument/status_test 944.7 ns/op 1305 B/op 22 allocs/op 948.5 ns/op 1305 B/op 22 allocs/op 1.00
BenchmarkDocument/status_test - ns/op 944.7 ns/op 948.5 ns/op 1.00
BenchmarkDocument/status_test - B/op 1305 B/op 1305 B/op 1
BenchmarkDocument/status_test - allocs/op 22 allocs/op 22 allocs/op 1
BenchmarkDocument/equals_test 7722 ns/op 7273 B/op 132 allocs/op 8425 ns/op 7273 B/op 132 allocs/op 0.92
BenchmarkDocument/equals_test - ns/op 7722 ns/op 8425 ns/op 0.92
BenchmarkDocument/equals_test - B/op 7273 B/op 7273 B/op 1
BenchmarkDocument/equals_test - allocs/op 132 allocs/op 132 allocs/op 1
BenchmarkDocument/nested_update_test 16848 ns/op 12139 B/op 262 allocs/op 17137 ns/op 12139 B/op 262 allocs/op 0.98
BenchmarkDocument/nested_update_test - ns/op 16848 ns/op 17137 ns/op 0.98
BenchmarkDocument/nested_update_test - B/op 12139 B/op 12139 B/op 1
BenchmarkDocument/nested_update_test - allocs/op 262 allocs/op 262 allocs/op 1
BenchmarkDocument/delete_test 22264 ns/op 15363 B/op 341 allocs/op 22521 ns/op 15364 B/op 341 allocs/op 0.99
BenchmarkDocument/delete_test - ns/op 22264 ns/op 22521 ns/op 0.99
BenchmarkDocument/delete_test - B/op 15363 B/op 15364 B/op 1.00
BenchmarkDocument/delete_test - allocs/op 341 allocs/op 341 allocs/op 1
BenchmarkDocument/object_test 8489 ns/op 6817 B/op 120 allocs/op 8675 ns/op 6817 B/op 120 allocs/op 0.98
BenchmarkDocument/object_test - ns/op 8489 ns/op 8675 ns/op 0.98
BenchmarkDocument/object_test - B/op 6817 B/op 6817 B/op 1
BenchmarkDocument/object_test - allocs/op 120 allocs/op 120 allocs/op 1
BenchmarkDocument/array_test 30987 ns/op 11947 B/op 276 allocs/op 29330 ns/op 11947 B/op 276 allocs/op 1.06
BenchmarkDocument/array_test - ns/op 30987 ns/op 29330 ns/op 1.06
BenchmarkDocument/array_test - B/op 11947 B/op 11947 B/op 1
BenchmarkDocument/array_test - allocs/op 276 allocs/op 276 allocs/op 1
BenchmarkDocument/text_test 30344 ns/op 14715 B/op 469 allocs/op 30880 ns/op 14715 B/op 469 allocs/op 0.98
BenchmarkDocument/text_test - ns/op 30344 ns/op 30880 ns/op 0.98
BenchmarkDocument/text_test - B/op 14715 B/op 14715 B/op 1
BenchmarkDocument/text_test - allocs/op 469 allocs/op 469 allocs/op 1
BenchmarkDocument/text_composition_test 29301 ns/op 18420 B/op 484 allocs/op 28996 ns/op 18423 B/op 484 allocs/op 1.01
BenchmarkDocument/text_composition_test - ns/op 29301 ns/op 28996 ns/op 1.01
BenchmarkDocument/text_composition_test - B/op 18420 B/op 18423 B/op 1.00
BenchmarkDocument/text_composition_test - allocs/op 484 allocs/op 484 allocs/op 1
BenchmarkDocument/rich_text_test 80494 ns/op 38476 B/op 1148 allocs/op 82181 ns/op 38477 B/op 1148 allocs/op 0.98
BenchmarkDocument/rich_text_test - ns/op 80494 ns/op 82181 ns/op 0.98
BenchmarkDocument/rich_text_test - B/op 38476 B/op 38477 B/op 1.00
BenchmarkDocument/rich_text_test - allocs/op 1148 allocs/op 1148 allocs/op 1
BenchmarkDocument/counter_test 17281 ns/op 10722 B/op 244 allocs/op 17406 ns/op 10722 B/op 244 allocs/op 0.99
BenchmarkDocument/counter_test - ns/op 17281 ns/op 17406 ns/op 0.99
BenchmarkDocument/counter_test - B/op 10722 B/op 10722 B/op 1
BenchmarkDocument/counter_test - allocs/op 244 allocs/op 244 allocs/op 1
BenchmarkDocument/text_edit_gc_100 1170890 ns/op 870853 B/op 16689 allocs/op 1189763 ns/op 870799 B/op 16689 allocs/op 0.98
BenchmarkDocument/text_edit_gc_100 - ns/op 1170890 ns/op 1189763 ns/op 0.98
BenchmarkDocument/text_edit_gc_100 - B/op 870853 B/op 870799 B/op 1.00
BenchmarkDocument/text_edit_gc_100 - allocs/op 16689 allocs/op 16689 allocs/op 1
BenchmarkDocument/text_edit_gc_1000 48362793 ns/op 50534959 B/op 181602 allocs/op 49279245 ns/op 50535306 B/op 181603 allocs/op 0.98
BenchmarkDocument/text_edit_gc_1000 - ns/op 48362793 ns/op 49279245 ns/op 0.98
BenchmarkDocument/text_edit_gc_1000 - B/op 50534959 B/op 50535306 B/op 1.00
BenchmarkDocument/text_edit_gc_1000 - allocs/op 181602 allocs/op 181603 allocs/op 1.00
BenchmarkDocument/text_split_gc_100 1781774 ns/op 1528684 B/op 15541 allocs/op 1804266 ns/op 1528651 B/op 15541 allocs/op 0.99
BenchmarkDocument/text_split_gc_100 - ns/op 1781774 ns/op 1804266 ns/op 0.99
BenchmarkDocument/text_split_gc_100 - B/op 1528684 B/op 1528651 B/op 1.00
BenchmarkDocument/text_split_gc_100 - allocs/op 15541 allocs/op 15541 allocs/op 1
BenchmarkDocument/text_split_gc_1000 111220721 ns/op 135076382 B/op 182090 allocs/op 115322288 ns/op 135076578 B/op 182089 allocs/op 0.96
BenchmarkDocument/text_split_gc_1000 - ns/op 111220721 ns/op 115322288 ns/op 0.96
BenchmarkDocument/text_split_gc_1000 - B/op 135076382 B/op 135076578 B/op 1.00
BenchmarkDocument/text_split_gc_1000 - allocs/op 182090 allocs/op 182089 allocs/op 1.00
BenchmarkDocument/text_delete_all_10000 15864211 ns/op 10181598 B/op 40671 allocs/op 16108905 ns/op 10183139 B/op 40675 allocs/op 0.98
BenchmarkDocument/text_delete_all_10000 - ns/op 15864211 ns/op 16108905 ns/op 0.98
BenchmarkDocument/text_delete_all_10000 - B/op 10181598 B/op 10183139 B/op 1.00
BenchmarkDocument/text_delete_all_10000 - allocs/op 40671 allocs/op 40675 allocs/op 1.00
BenchmarkDocument/text_delete_all_100000 290759738 ns/op 142691340 B/op 411742 allocs/op 281843792 ns/op 142673256 B/op 411662 allocs/op 1.03
BenchmarkDocument/text_delete_all_100000 - ns/op 290759738 ns/op 281843792 ns/op 1.03
BenchmarkDocument/text_delete_all_100000 - B/op 142691340 B/op 142673256 B/op 1.00
BenchmarkDocument/text_delete_all_100000 - allocs/op 411742 allocs/op 411662 allocs/op 1.00
BenchmarkDocument/text_100 216232 ns/op 120037 B/op 5081 allocs/op 217899 ns/op 120035 B/op 5081 allocs/op 0.99
BenchmarkDocument/text_100 - ns/op 216232 ns/op 217899 ns/op 0.99
BenchmarkDocument/text_100 - B/op 120037 B/op 120035 B/op 1.00
BenchmarkDocument/text_100 - allocs/op 5081 allocs/op 5081 allocs/op 1
BenchmarkDocument/text_1000 2346536 ns/op 1169024 B/op 50085 allocs/op 2395184 ns/op 1169023 B/op 50085 allocs/op 0.98
BenchmarkDocument/text_1000 - ns/op 2346536 ns/op 2395184 ns/op 0.98
BenchmarkDocument/text_1000 - B/op 1169024 B/op 1169023 B/op 1.00
BenchmarkDocument/text_1000 - allocs/op 50085 allocs/op 50085 allocs/op 1
BenchmarkDocument/array_1000 1207658 ns/op 1091357 B/op 11831 allocs/op 1216290 ns/op 1091323 B/op 11831 allocs/op 0.99
BenchmarkDocument/array_1000 - ns/op 1207658 ns/op 1216290 ns/op 0.99
BenchmarkDocument/array_1000 - B/op 1091357 B/op 1091323 B/op 1.00
BenchmarkDocument/array_1000 - allocs/op 11831 allocs/op 11831 allocs/op 1
BenchmarkDocument/array_10000 13311191 ns/op 9800144 B/op 120296 allocs/op 13529767 ns/op 9799431 B/op 120295 allocs/op 0.98
BenchmarkDocument/array_10000 - ns/op 13311191 ns/op 13529767 ns/op 0.98
BenchmarkDocument/array_10000 - B/op 9800144 B/op 9799431 B/op 1.00
BenchmarkDocument/array_10000 - allocs/op 120296 allocs/op 120295 allocs/op 1.00
BenchmarkDocument/array_gc_100 149827 ns/op 132704 B/op 1260 allocs/op 147319 ns/op 132701 B/op 1260 allocs/op 1.02
BenchmarkDocument/array_gc_100 - ns/op 149827 ns/op 147319 ns/op 1.02
BenchmarkDocument/array_gc_100 - B/op 132704 B/op 132701 B/op 1.00
BenchmarkDocument/array_gc_100 - allocs/op 1260 allocs/op 1260 allocs/op 1
BenchmarkDocument/array_gc_1000 1419969 ns/op 1159215 B/op 12877 allocs/op 1391804 ns/op 1159161 B/op 12876 allocs/op 1.02
BenchmarkDocument/array_gc_1000 - ns/op 1419969 ns/op 1391804 ns/op 1.02
BenchmarkDocument/array_gc_1000 - B/op 1159215 B/op 1159161 B/op 1.00
BenchmarkDocument/array_gc_1000 - allocs/op 12877 allocs/op 12876 allocs/op 1.00
BenchmarkDocument/counter_1000 198485 ns/op 193081 B/op 5771 allocs/op 202278 ns/op 193079 B/op 5771 allocs/op 0.98
BenchmarkDocument/counter_1000 - ns/op 198485 ns/op 202278 ns/op 0.98
BenchmarkDocument/counter_1000 - B/op 193081 B/op 193079 B/op 1.00
BenchmarkDocument/counter_1000 - allocs/op 5771 allocs/op 5771 allocs/op 1
BenchmarkDocument/counter_10000 2138272 ns/op 2087995 B/op 59778 allocs/op 2179350 ns/op 2087994 B/op 59778 allocs/op 0.98
BenchmarkDocument/counter_10000 - ns/op 2138272 ns/op 2179350 ns/op 0.98
BenchmarkDocument/counter_10000 - B/op 2087995 B/op 2087994 B/op 1.00
BenchmarkDocument/counter_10000 - allocs/op 59778 allocs/op 59778 allocs/op 1
BenchmarkDocument/object_1000 1336280 ns/op 1428169 B/op 9849 allocs/op 1358218 ns/op 1428338 B/op 9849 allocs/op 0.98
BenchmarkDocument/object_1000 - ns/op 1336280 ns/op 1358218 ns/op 0.98
BenchmarkDocument/object_1000 - B/op 1428169 B/op 1428338 B/op 1.00
BenchmarkDocument/object_1000 - allocs/op 9849 allocs/op 9849 allocs/op 1
BenchmarkDocument/object_10000 14765484 ns/op 12165817 B/op 100560 allocs/op 15101354 ns/op 12169634 B/op 100574 allocs/op 0.98
BenchmarkDocument/object_10000 - ns/op 14765484 ns/op 15101354 ns/op 0.98
BenchmarkDocument/object_10000 - B/op 12165817 B/op 12169634 B/op 1.00
BenchmarkDocument/object_10000 - allocs/op 100560 allocs/op 100574 allocs/op 1.00
BenchmarkDocument/tree_100 1092394 ns/op 943703 B/op 6101 allocs/op 1065040 ns/op 943710 B/op 6101 allocs/op 1.03
BenchmarkDocument/tree_100 - ns/op 1092394 ns/op 1065040 ns/op 1.03
BenchmarkDocument/tree_100 - B/op 943703 B/op 943710 B/op 1.00
BenchmarkDocument/tree_100 - allocs/op 6101 allocs/op 6101 allocs/op 1
BenchmarkDocument/tree_1000 75533416 ns/op 86460453 B/op 60115 allocs/op 79403008 ns/op 86460914 B/op 60115 allocs/op 0.95
BenchmarkDocument/tree_1000 - ns/op 75533416 ns/op 79403008 ns/op 0.95
BenchmarkDocument/tree_1000 - B/op 86460453 B/op 86460914 B/op 1.00
BenchmarkDocument/tree_1000 - allocs/op 60115 allocs/op 60115 allocs/op 1
BenchmarkDocument/tree_10000 9345340687 ns/op 8580672176 B/op 600244 allocs/op 9599675702 ns/op 8580652352 B/op 600218 allocs/op 0.97
BenchmarkDocument/tree_10000 - ns/op 9345340687 ns/op 9599675702 ns/op 0.97
BenchmarkDocument/tree_10000 - B/op 8580672176 B/op 8580652352 B/op 1.00
BenchmarkDocument/tree_10000 - allocs/op 600244 allocs/op 600218 allocs/op 1.00
BenchmarkDocument/tree_delete_all_1000 72502755 ns/op 87509903 B/op 75265 allocs/op 74807034 ns/op 87510233 B/op 75266 allocs/op 0.97
BenchmarkDocument/tree_delete_all_1000 - ns/op 72502755 ns/op 74807034 ns/op 0.97
BenchmarkDocument/tree_delete_all_1000 - B/op 87509903 B/op 87510233 B/op 1.00
BenchmarkDocument/tree_delete_all_1000 - allocs/op 75265 allocs/op 75266 allocs/op 1.00
BenchmarkDocument/tree_edit_gc_100 3708663 ns/op 4147776 B/op 15141 allocs/op 3758081 ns/op 4146743 B/op 15141 allocs/op 0.99
BenchmarkDocument/tree_edit_gc_100 - ns/op 3708663 ns/op 3758081 ns/op 0.99
BenchmarkDocument/tree_edit_gc_100 - B/op 4147776 B/op 4146743 B/op 1.00
BenchmarkDocument/tree_edit_gc_100 - allocs/op 15141 allocs/op 15141 allocs/op 1
BenchmarkDocument/tree_edit_gc_1000 297071988 ns/op 383744304 B/op 154841 allocs/op 297491718 ns/op 383745070 B/op 154850 allocs/op 1.00
BenchmarkDocument/tree_edit_gc_1000 - ns/op 297071988 ns/op 297491718 ns/op 1.00
BenchmarkDocument/tree_edit_gc_1000 - B/op 383744304 B/op 383745070 B/op 1.00
BenchmarkDocument/tree_edit_gc_1000 - allocs/op 154841 allocs/op 154850 allocs/op 1.00
BenchmarkDocument/tree_split_gc_100 2462022 ns/op 2412584 B/op 11125 allocs/op 2515241 ns/op 2412499 B/op 11125 allocs/op 0.98
BenchmarkDocument/tree_split_gc_100 - ns/op 2462022 ns/op 2515241 ns/op 0.98
BenchmarkDocument/tree_split_gc_100 - B/op 2412584 B/op 2412499 B/op 1.00
BenchmarkDocument/tree_split_gc_100 - allocs/op 11125 allocs/op 11125 allocs/op 1
BenchmarkDocument/tree_split_gc_1000 179437770 ns/op 222251964 B/op 121993 allocs/op 183279595 ns/op 222251146 B/op 121990 allocs/op 0.98
BenchmarkDocument/tree_split_gc_1000 - ns/op 179437770 ns/op 183279595 ns/op 0.98
BenchmarkDocument/tree_split_gc_1000 - B/op 222251964 B/op 222251146 B/op 1.00
BenchmarkDocument/tree_split_gc_1000 - allocs/op 121993 allocs/op 121990 allocs/op 1.00
BenchmarkRPC/client_to_server 369481422 ns/op 16972112 B/op 175382 allocs/op 380944463 ns/op 16827440 B/op 174540 allocs/op 0.97
BenchmarkRPC/client_to_server - ns/op 369481422 ns/op 380944463 ns/op 0.97
BenchmarkRPC/client_to_server - B/op 16972112 B/op 16827440 B/op 1.01
BenchmarkRPC/client_to_server - allocs/op 175382 allocs/op 174540 allocs/op 1.00
BenchmarkRPC/client_to_client_via_server 622781656 ns/op 33804800 B/op 320562 allocs/op 639027950 ns/op 33199144 B/op 319499 allocs/op 0.97
BenchmarkRPC/client_to_client_via_server - ns/op 622781656 ns/op 639027950 ns/op 0.97
BenchmarkRPC/client_to_client_via_server - B/op 33804800 B/op 33199144 B/op 1.02
BenchmarkRPC/client_to_client_via_server - allocs/op 320562 allocs/op 319499 allocs/op 1.00
BenchmarkRPC/attach_large_document 1660314898 ns/op 1919903560 B/op 8858 allocs/op 1527372072 ns/op 1895497176 B/op 9544 allocs/op 1.09
BenchmarkRPC/attach_large_document - ns/op 1660314898 ns/op 1527372072 ns/op 1.09
BenchmarkRPC/attach_large_document - B/op 1919903560 B/op 1895497176 B/op 1.01
BenchmarkRPC/attach_large_document - allocs/op 8858 allocs/op 9544 allocs/op 0.93
BenchmarkRPC/adminCli_to_server 538626234 ns/op 35964920 B/op 289546 allocs/op 548632167 ns/op 35955872 B/op 289508 allocs/op 0.98
BenchmarkRPC/adminCli_to_server - ns/op 538626234 ns/op 548632167 ns/op 0.98
BenchmarkRPC/adminCli_to_server - B/op 35964920 B/op 35955872 B/op 1.00
BenchmarkRPC/adminCli_to_server - allocs/op 289546 allocs/op 289508 allocs/op 1.00
BenchmarkLocker 63.27 ns/op 16 B/op 1 allocs/op 62.91 ns/op 16 B/op 1 allocs/op 1.01
BenchmarkLocker - ns/op 63.27 ns/op 62.91 ns/op 1.01
BenchmarkLocker - B/op 16 B/op 16 B/op 1
BenchmarkLocker - allocs/op 1 allocs/op 1 allocs/op 1
BenchmarkLockerParallel 38.39 ns/op 0 B/op 0 allocs/op 38.7 ns/op 0 B/op 0 allocs/op 0.99
BenchmarkLockerParallel - ns/op 38.39 ns/op 38.7 ns/op 0.99
BenchmarkLockerParallel - B/op 0 B/op 0 B/op 1
BenchmarkLockerParallel - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkLockerMoreKeys 148.2 ns/op 15 B/op 0 allocs/op 145.7 ns/op 15 B/op 0 allocs/op 1.02
BenchmarkLockerMoreKeys - ns/op 148.2 ns/op 145.7 ns/op 1.02
BenchmarkLockerMoreKeys - B/op 15 B/op 15 B/op 1
BenchmarkLockerMoreKeys - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkChange/Push_10_Changes 3864785 ns/op 121676 B/op 1285 allocs/op 4030347 ns/op 121130 B/op 1284 allocs/op 0.96
BenchmarkChange/Push_10_Changes - ns/op 3864785 ns/op 4030347 ns/op 0.96
BenchmarkChange/Push_10_Changes - B/op 121676 B/op 121130 B/op 1.00
BenchmarkChange/Push_10_Changes - allocs/op 1285 allocs/op 1284 allocs/op 1.00
BenchmarkChange/Push_100_Changes 14432384 ns/op 572169 B/op 6654 allocs/op 14870554 ns/op 568236 B/op 6654 allocs/op 0.97
BenchmarkChange/Push_100_Changes - ns/op 14432384 ns/op 14870554 ns/op 0.97
BenchmarkChange/Push_100_Changes - B/op 572169 B/op 568236 B/op 1.01
BenchmarkChange/Push_100_Changes - allocs/op 6654 allocs/op 6654 allocs/op 1
BenchmarkChange/Push_1000_Changes 116512027 ns/op 5273851 B/op 63149 allocs/op 119288507 ns/op 5303693 B/op 63151 allocs/op 0.98
BenchmarkChange/Push_1000_Changes - ns/op 116512027 ns/op 119288507 ns/op 0.98
BenchmarkChange/Push_1000_Changes - B/op 5273851 B/op 5303693 B/op 0.99
BenchmarkChange/Push_1000_Changes - allocs/op 63149 allocs/op 63151 allocs/op 1.00
BenchmarkChange/Pull_10_Changes 2878843 ns/op 100758 B/op 1004 allocs/op 2939071 ns/op 100887 B/op 1005 allocs/op 0.98
BenchmarkChange/Pull_10_Changes - ns/op 2878843 ns/op 2939071 ns/op 0.98
BenchmarkChange/Pull_10_Changes - B/op 100758 B/op 100887 B/op 1.00
BenchmarkChange/Pull_10_Changes - allocs/op 1004 allocs/op 1005 allocs/op 1.00
BenchmarkChange/Pull_100_Changes 4290050 ns/op 266131 B/op 3474 allocs/op 4402682 ns/op 266094 B/op 3475 allocs/op 0.97
BenchmarkChange/Pull_100_Changes - ns/op 4290050 ns/op 4402682 ns/op 0.97
BenchmarkChange/Pull_100_Changes - B/op 266131 B/op 266094 B/op 1.00
BenchmarkChange/Pull_100_Changes - allocs/op 3474 allocs/op 3475 allocs/op 1.00
BenchmarkChange/Pull_1000_Changes 8505860 ns/op 1490606 B/op 29856 allocs/op 8716095 ns/op 1490666 B/op 29859 allocs/op 0.98
BenchmarkChange/Pull_1000_Changes - ns/op 8505860 ns/op 8716095 ns/op 0.98
BenchmarkChange/Pull_1000_Changes - B/op 1490606 B/op 1490666 B/op 1.00
BenchmarkChange/Pull_1000_Changes - allocs/op 29856 allocs/op 29859 allocs/op 1.00
BenchmarkSnapshot/Push_3KB_snapshot 16776665 ns/op 716197 B/op 6655 allocs/op 17429568 ns/op 710905 B/op 6659 allocs/op 0.96
BenchmarkSnapshot/Push_3KB_snapshot - ns/op 16776665 ns/op 17429568 ns/op 0.96
BenchmarkSnapshot/Push_3KB_snapshot - B/op 716197 B/op 710905 B/op 1.01
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op 6655 allocs/op 6659 allocs/op 1.00
BenchmarkSnapshot/Push_30KB_snapshot 119400297 ns/op 5677067 B/op 63159 allocs/op 122701692 ns/op 5723387 B/op 63155 allocs/op 0.97
BenchmarkSnapshot/Push_30KB_snapshot - ns/op 119400297 ns/op 122701692 ns/op 0.97
BenchmarkSnapshot/Push_30KB_snapshot - B/op 5677067 B/op 5723387 B/op 0.99
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op 63159 allocs/op 63155 allocs/op 1.00
BenchmarkSnapshot/Pull_3KB_snapshot 6391365 ns/op 922571 B/op 15511 allocs/op 6452078 ns/op 922370 B/op 15513 allocs/op 0.99
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op 6391365 ns/op 6452078 ns/op 0.99
BenchmarkSnapshot/Pull_3KB_snapshot - B/op 922571 B/op 922370 B/op 1.00
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op 15511 allocs/op 15513 allocs/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot 15402247 ns/op 7151539 B/op 150107 allocs/op 15725581 ns/op 7148961 B/op 150104 allocs/op 0.98
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op 15402247 ns/op 15725581 ns/op 0.98
BenchmarkSnapshot/Pull_30KB_snapshot - B/op 7151539 B/op 7148961 B/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op 150107 allocs/op 150104 allocs/op 1.00
BenchmarkSync/memory_sync_10_test 6675 ns/op 1286 B/op 38 allocs/op 7948 ns/op 1286 B/op 38 allocs/op 0.84
BenchmarkSync/memory_sync_10_test - ns/op 6675 ns/op 7948 ns/op 0.84
BenchmarkSync/memory_sync_10_test - B/op 1286 B/op 1286 B/op 1
BenchmarkSync/memory_sync_10_test - allocs/op 38 allocs/op 38 allocs/op 1
BenchmarkSync/memory_sync_100_test 50024 ns/op 8652 B/op 274 allocs/op 57600 ns/op 8982 B/op 294 allocs/op 0.87
BenchmarkSync/memory_sync_100_test - ns/op 50024 ns/op 57600 ns/op 0.87
BenchmarkSync/memory_sync_100_test - B/op 8652 B/op 8982 B/op 0.96
BenchmarkSync/memory_sync_100_test - allocs/op 274 allocs/op 294 allocs/op 0.93
BenchmarkSync/memory_sync_1000_test 578683 ns/op 73909 B/op 2094 allocs/op 557376 ns/op 75835 B/op 2215 allocs/op 1.04
BenchmarkSync/memory_sync_1000_test - ns/op 578683 ns/op 557376 ns/op 1.04
BenchmarkSync/memory_sync_1000_test - B/op 73909 B/op 75835 B/op 0.97
BenchmarkSync/memory_sync_1000_test - allocs/op 2094 allocs/op 2215 allocs/op 0.95
BenchmarkSync/memory_sync_10000_test 7249781 ns/op 738283 B/op 20256 allocs/op 7294306 ns/op 734206 B/op 20243 allocs/op 0.99
BenchmarkSync/memory_sync_10000_test - ns/op 7249781 ns/op 7294306 ns/op 0.99
BenchmarkSync/memory_sync_10000_test - B/op 738283 B/op 734206 B/op 1.01
BenchmarkSync/memory_sync_10000_test - allocs/op 20256 allocs/op 20243 allocs/op 1.00
BenchmarkTextEditing 5035868680 ns/op 3901912864 B/op 18743459 allocs/op 5329256405 ns/op 3901959024 B/op 18743575 allocs/op 0.94
BenchmarkTextEditing - ns/op 5035868680 ns/op 5329256405 ns/op 0.94
BenchmarkTextEditing - B/op 3901912864 B/op 3901959024 B/op 1.00
BenchmarkTextEditing - allocs/op 18743459 allocs/op 18743575 allocs/op 1.00

This comment was automatically generated by workflow using github-action-benchmark.

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 78.72340% with 10 lines in your changes missing coverage. Please review.

Project coverage is 50.74%. Comparing base (e4885cb) to head (e8a90a7).

Files Patch % Lines
server/backend/database/client_info.go 46.15% 5 Missing and 2 partials ⚠️
server/backend/database/memory/database.go 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #907      +/-   ##
==========================================
+ Coverage   50.57%   50.74%   +0.17%     
==========================================
  Files          70       70              
  Lines       10508    10551      +43     
==========================================
+ Hits         5314     5354      +40     
  Misses       4666     4666              
- Partials      528      531       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@raararaara raararaara marked this pull request as ready for review June 27, 2024 08:48
@raararaara raararaara marked this pull request as draft June 27, 2024 08:49
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range and nitpick comments (2)
server/clients/clients.go (2)

Line range hint 57-81: Ensure robust error handling and document the "continue" decision.

The loop iterates over documents, detaching them if attached. However, if a document is not attached (isAttached is false), the code simply continues. This decision should be documented to clarify why it's safe to ignore these documents, especially since the PR is focused on ensuring documents are detached correctly.

+ // If the document is not attached, skip to the next document.
  if !isAttached {
      continue
  }

Line range hint 83-107: Ensure correct error handling and update flow.

The function concludes by deactivating the client and updating the client info in the database. Ensure that the error handling is robust and that the updatedClientInfo is correctly used in the subsequent code. The TODO comment about removing the client's presence from the document suggests additional work is needed. This should be tracked in a separate issue if not addressed in this PR.

+ // TODO: Implement removal of client's presence from the document as noted.

Would you like me to open a GitHub issue to track the implementation of the TODO regarding the removal of the client's presence?

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f86a4f3 and e607d8d.

Files selected for processing (3)
  • server/backend/database/client_info.go (2 hunks)
  • server/backend/database/client_info_test.go (1 hunks)
  • server/clients/clients.go (3 hunks)
Additional comments not posted (2)
server/clients/clients.go (1)

78-81: Check error handling in database update operations.

The function db.UpdateClientInfoAfterPushPull updates the client info after detaching a document. Ensure that this operation is necessary at this point and consider adding a comment to explain why this operation is performed after each document detachment, as it could potentially be optimized.

server/backend/database/client_info_test.go (1)

148-164: Good coverage for the deactivation scenario, consider adding more context.

The test effectively checks if documents remain attached after deactivation, which is a key part of the PR's objectives. Consider adding a comment explaining why ErrAttachedDocumentExists is the expected error, as this helps clarify the test's purpose.

server/backend/database/client_info.go Outdated Show resolved Hide resolved
@raararaara raararaara marked this pull request as ready for review June 27, 2024 09:38
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e607d8d and 24f8120.

Files selected for processing (2)
  • server/backend/database/client_info.go (2 hunks)
  • server/backend/database/client_info_test.go (1 hunks)
Additional comments not posted (2)
server/backend/database/client_info_test.go (1)

149-164: Review of New Test Case for Document Detachment

The new test case "document detached when client deactivate test" effectively simulates the scenario where a document is attached and then checked for detachment upon client deactivation. The use of EnsureDocumentsDetachedWhenDeactivated and the assertion for ErrAttachedDocumentExists align well with the PR's objectives to handle document detachment errors correctly.

However, it seems the test expects an error to be thrown when documents should be detached, which might be confusing. Consider clarifying whether the test should expect no error (i.e., all documents are detached successfully) or if the current behavior is intentional.

server/backend/database/client_info.go (1)

35-35: New Error Definition Reviewed

The introduction of ErrAttachedDocumentExists is well-placed within the context of the PR's objectives. This error provides a clear description of the issue when a document remains attached post-deactivation, aiding in precise error handling and user feedback.

server/backend/database/client_info.go Outdated Show resolved Hide resolved
@hackerwins hackerwins marked this pull request as draft June 28, 2024 06:17
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 24f8120 and 5d9cf0d.

Files selected for processing (1)
  • server/backend/database/testcases/testcases.go (2 hunks)
Additional context used
golangci-lint
server/backend/database/testcases/testcases.go

24-24: File is not goimports-ed with -local github.com/yorkie-team/yorkie

(goimports)

@hackerwins hackerwins removed their request for review June 28, 2024 09:07
@raararaara raararaara marked this pull request as ready for review July 2, 2024 09:34
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5d9cf0d and 7b5cca6.

Files selected for processing (6)
  • server/backend/database/client_info.go (2 hunks)
  • server/backend/database/client_info_test.go (1 hunks)
  • server/backend/database/memory/database.go (1 hunks)
  • server/backend/database/mongo/client.go (1 hunks)
  • server/backend/database/testcases/testcases.go (1 hunks)
  • server/clients/clients.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
  • server/backend/database/client_info.go
  • server/backend/database/client_info_test.go
  • server/backend/database/testcases/testcases.go
Additional comments not posted (3)
server/clients/clients.go (1)

Line range hint 52-76: Verify the detachment logic.

Ensure that the detachment logic correctly handles all cases and that documents are properly detached when a client is deactivated. Consider edge cases where documents may not be attached or already detached.

Verification successful

Line range hint 52-76: Verify the detachment logic.

The Deactivate function in server/clients/clients.go calls db.DeactivateClient, which handles the detachment of documents. The DeactivateClient function in server/backend/database/mongo/client.go and server/backend/database/memory/database.go includes logic to set document statuses to "detached" during client deactivation. This ensures that documents are properly detached when a client is deactivated.

  • server/clients/clients.go: The Deactivate function.
  • server/backend/database/mongo/client.go: The DeactivateClient function.
  • server/backend/database/memory/database.go: The DeactivateClient function.

The detachment logic is correctly implemented in the DeactivateClient function, ensuring documents are detached when a client is deactivated.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the detachment logic in the Deactivate function.

# Test: Check for edge cases and ensure documents are detached correctly.
rg --type go 'DeactivateClient' | rg 'DetachDocument'

Length of output: 51



Script:

#!/bin/bash
# Description: Verify the detachment logic in the Deactivate function.

# Find the definition of the DeactivateClient function to understand its implementation.
ast-grep --lang go --pattern 'func DeactivateClient($_, $_) ($_ , $_) {
  $$$
}' --json

# Search for any detachment-related logic within the DeactivateClient function.
rg 'DeactivateClient' -A 20 | rg -i 'detach|remove|delete'

Length of output: 10410



Script:

#!/bin/bash
# Description: Verify the detachment logic in the Deactivate function.

# Find the definition of the DeactivateClient function to understand its implementation.
rg 'func DeactivateClient' -A 50

# Search for any detachment-related logic within the DeactivateClient function.
rg 'DeactivateClient' -A 50 | rg -i 'detach|remove|delete'

Length of output: 13891



Script:

#!/bin/bash
# Description: Verify the detachment logic in the Deactivate function specifically in the clients.go file.

# Find the definition of the DeactivateClient function in the server/clients/clients.go file.
rg 'func DeactivateClient' server/clients/clients.go -A 50

Length of output: 60



Script:

#!/bin/bash
# Description: Verify the detachment logic in the Deactivate function.

# Find the definition of the DeactivateClient function across all Go files in the repository.
rg 'func DeactivateClient' -t go -A 50

Length of output: 40



Script:

#!/bin/bash
# Description: Verify the detachment logic in the Deactivate function in the server/clients/clients.go file.

# Find the definition of the Deactivate function in the server/clients/clients.go file.
rg 'func Deactivate' server/clients/clients.go -A 50

Length of output: 1273



Script:

#!/bin/bash
# Description: Verify the detachment logic in the DeactivateClient function.

# Find the definition of the DeactivateClient function across all Go files in the repository.
rg 'func DeactivateClient' -t go -A 50

Length of output: 40



Script:

#!/bin/bash
# Description: Verify the detachment logic in the DeactivateClient function or related logic in the database files.

# Find the definition of the DeactivateClient function or related logic in the server/backend/database directory.
rg 'DeactivateClient' server/backend/database -A 50

# Find the definition of the Deactivate function in the server/backend/database directory.
rg 'Deactivate' server/backend/database -A 50

Length of output: 154159

server/backend/database/memory/database.go (1)

526-534: Verify the detachment logic.

Ensure that the detachment logic correctly handles all cases and that documents are properly detached when a client is deactivated. Consider edge cases where documents may not be attached or already detached.

server/backend/database/mongo/client.go (1)

523-523: Add function-level documentation.

Adding a brief description of what the function does, its parameters, and return values will improve readability and maintainability.

// DeactivateClient deactivates the client of the given refKey and updates document statuses as detached.
// Parameters:
// - ctx: context for the operation
// - refKey: reference key of the client
// Returns:
// - *database.ClientInfo: information about the deactivated client
// - error: error if any occurred during the operation

server/backend/database/mongo/client.go Outdated Show resolved Hide resolved
@raararaara raararaara marked this pull request as draft July 2, 2024 09:45
@raararaara raararaara marked this pull request as ready for review July 2, 2024 10:14
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7b5cca6 and cc89fbf.

Files selected for processing (1)
  • server/backend/database/mongo/client.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • server/backend/database/mongo/client.go

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cc89fbf and 6eda7d6.

Files selected for processing (1)
  • server/backend/database/mongo/client.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • server/backend/database/mongo/client.go

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I left a few comments.

server/backend/database/testcases/testcases.go Outdated Show resolved Hide resolved
server/clients/clients.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
server/clients/clients.go (1)

68-71: Optimize the update of SyncedSeq.

The current approach is similar to an n+1 query problem. Investigate the possibility of optimizing this process by using a single query.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6eda7d6 and a8849b3.

Files selected for processing (2)
  • server/backend/database/testcases/testcases.go (1 hunks)
  • server/clients/clients.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • server/backend/database/testcases/testcases.go
Additional comments not posted (3)
server/clients/clients.go (3)

52-55: Clarify the TODO comment.

The TODO comment should be more specific about how to remove the presence of the client from the document.

-  // TODO(hackerwins): We need to remove the presence of the client from the document.
+  // TODO(hackerwins): Implement logic to remove the presence of the client from the document.

57-62: Acknowledge added comments for clarity.

The added comments provide clarity on the process and challenges of updating ClientInfo and SyncedSeq.


63-63: Ensure error handling for DeactivateClient.

Consider adding more specific error handling for the DeactivateClient call to differentiate between types of errors (e.g., network issues, database issues).

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a8849b3 and e8a90a7.

Files selected for processing (1)
  • server/clients/clients.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • server/clients/clients.go

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

@hackerwins hackerwins changed the title Fix issue with document detachment not being reflected in database upon client Fix missing document detachments when client is deactivated Jul 3, 2024
@hackerwins hackerwins merged commit c658096 into main Jul 3, 2024
4 checks passed
@hackerwins hackerwins deleted the fix-remove-document branch July 3, 2024 02:05
hackerwins pushed a commit that referenced this pull request Nov 21, 2024
)

This change introduces a migration script to handle documents that remain
attached to deactivated clients from before PRs #907 and #1036. Instead of
direct detachment, the script temporarily reactivates clients to leverage the
existing housekeeping process.

The approach:
- Identifies documents still attached to deactivated clients
- Updates client states from deactivated to activated
- Lets housekeeping process handle document detachment naturally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants