Optimize document detachment in Cluster Server #1055

raararaara · 2024-10-31T05:24:44Z

What this PR does / why we need it?

This PR optimizes the document detach operation through the Cluster Server. As mentioned in #1039 , improvements during the client deactivation process highlighted several areas for enhancement, particularly the part that generates the presence clear event, which was the most time-consuming.

Previously, the process involved building the document corresponding to the client's checkpoint to create changes. This approach was time-intensive due to the overhead of document construction. To address this, we have introduced logic to generate changes directly, bypassing the document build phase.

The changes that are generated will be considered as the latest change created immediately after the targeted client (or actor) has its latestChangeInfo.
To quickly obtain the latestChangeInfo for a specific actor, the index of the change collection has been added, with fields { doc_id, project_id, actor_id, server_seq }.

By implementing these changes, we have observed significant improvements: the execution time for documents that previously took about 150ms for detachment has been reduced to an average of 30ms.

Any background context you want to provide?

Background: The optimization covered in this PR is based on the contents of PR Detach documents when client is deactivated #1036, which covers the overall flow of client deactivation, and issue Optimize the Client Deactivate flow to enhance performance #1039 , which covers points that may become bottlenecks in the process.
Limitation: It’s important to note that the Lamport timestamp and version vector for the created change will differ from the actual Lamport timestamp and version vector associated with the client’s document, which could lead to potential issues, further discussed in another issue.

Which issue(s) this PR fixes:

Addresses #1039

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation:

Checklist:

Added relevant tests or not required
Didn't break anything

Summary by CodeRabbit

Release Notes

New Features
- Introduced a method to retrieve the latest change information by actor ID for improved tracking of document changes.
- Added new indexes to enhance query efficiency for change retrieval.
Bug Fixes
- Optimized the document detachment process in the cluster server for better performance.
Tests
- Expanded test coverage with new test cases for validating the retrieval of the latest change information.

These updates enhance the overall functionality and performance of the application, particularly in tracking changes and managing document states.

coderabbitai · 2024-10-31T05:24:52Z

Caution

Review failed

The pull request is closed.

Walkthrough

The changes in this pull request introduce a new method FindLatestChangeInfoByActor across multiple database implementations, including the Database interface, in-memory database, and MongoDB client. This method allows for retrieving the latest change information associated with a specific actor ID and document reference. Additionally, new indexes are added to optimize queries, and corresponding test cases are included to validate the new functionality. The DetachDocument method in the cluster server is also modified to enhance efficiency in handling document changes.

Changes

File Path	Change Summary
`server/backend/database/database.go`	Added method `FindLatestChangeInfoByActor(ctx context.Context, docRefKey types.DocRefKey, actorID types.ID, serverSeq int64) (*ChangeInfo, error)` to `Database` interface.
`server/backend/database/memory/database.go`	Added method `func (d DB) FindLatestChangeInfoByActor(ctx context.Context, docRefKey types.DocRefKey, actorID types.ID, serverSeq int64) (database.ChangeInfo, error)` to `DB` struct.
`server/backend/database/memory/database_test.go`	Added test case `RunFindLatestChangeInfoTest` in `TestDB` function to validate the retrieval of the latest change information.
`server/backend/database/memory/indexes.go`	Added new index `doc_id_actor_id_server_seq` to `tblChanges` table schema.
`server/backend/database/mongo/client.go`	Added method `func (c Client) FindLatestChangeInfoByActor(ctx context.Context, docRefKey types.DocRefKey, actorID types.ID, serverSeq int64) (database.ChangeInfo, error)` to `Client` struct.
`server/backend/database/mongo/client_test.go`	Added test case `RunFindLatestChangeInfoTest(t, cli, dummyProjectID)` in `TestClient` function to check latest change information retrieval.
`server/backend/database/mongo/indexes.go`	Added new index for `ColChanges` with keys `doc_id`, `project_id`, `actor_id`, `server_seq` set to unique.
`server/backend/database/testcases/testcases.go`	Added test function `RunFindLatestChangeInfoTest` to validate latest change retrieval by actor for a document.
`server/rpc/cluster_server.go`	Updated `DetachDocument` method to optimize change pack creation and document detachment logic, replacing expensive operations with more efficient ones.

Possibly related issues

Optimize the Client Deactivate flow to enhance performance #1039: The changes in this PR optimize the DetachDocument method, which relates to improving the ChangePack creation logic, addressing performance issues identified in the issue.

Possibly related PRs

Enhance GetDocuments API by adding bulk retrieval #931: This PR adds FindDocInfosByKeys, enhancing database retrieval capabilities similar to FindLatestChangeInfoByActor.
Optimize FindChangeInfosBetweenServerSeqs to prevent unnecessary Query #974: This PR optimizes FindChangeInfosBetweenServerSeqs, relevant to the change information retrieval introduced here.
Add consistency test for ClientInfo update failure in PushPull #1000: Introduces a consistency test for ClientInfo, which may interact with the change information retrieval methods.
Fix transaction in UpdateAndFindMinSyncedVersionVector #1050: Modifies UpdateAndFindMinSyncedVersionVector, related to managing version vectors and change information.
Introduce cmap for distributing mutexes per documents #1051: Introduces a concurrent map for mutexes per document, potentially enhancing performance for change information retrieval.

Suggested labels

enhancement 🌟

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>, please review it.
- 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @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 using 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.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration 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.

codecov · 2024-10-31T05:31:45Z

Codecov Report

Attention: Patch coverage is 50.79365% with 31 lines in your changes missing coverage. Please review.

Project coverage is 46.82%. Comparing base (4485a28) to head (c3f9542).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
server/rpc/cluster_server.go	0.00%	18 Missing ⚠️
server/backend/database/mongo/client.go	64.00%	6 Missing and 3 partials ⚠️
server/backend/database/memory/database.go	80.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1055      +/-   ##
==========================================
+ Coverage   46.76%   46.82%   +0.05%     
==========================================
  Files          82       84       +2     
  Lines       12001    12180     +179     
==========================================
+ Hits         5612     5703      +91     
- Misses       5826     5909      +83     
- Partials      563      568       +5

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

github-actions

Go Benchmark

Benchmark suite	Current: `c3f9542`	Previous: `a8605ef`	Ratio
`BenchmarkDocument/constructor_test`	`1502` ns/op 1337 B/op 24 allocs/op	`1502` ns/op 1337 B/op 24 allocs/op	`1`
`BenchmarkDocument/constructor_test - ns/op`	`1502` ns/op	`1502` ns/op	`1`
`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`	`964` ns/op 1305 B/op 22 allocs/op	`962.5` ns/op 1305 B/op 22 allocs/op	`1.00`
`BenchmarkDocument/status_test - ns/op`	`964` ns/op	`962.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`	`7809` ns/op 7529 B/op 134 allocs/op	`7816` ns/op 7529 B/op 134 allocs/op	`1.00`
`BenchmarkDocument/equals_test - ns/op`	`7809` ns/op	`7816` ns/op	`1.00`
`BenchmarkDocument/equals_test - B/op`	`7529` B/op	`7529` B/op	`1`
`BenchmarkDocument/equals_test - allocs/op`	`134` allocs/op	`134` allocs/op	`1`
`BenchmarkDocument/nested_update_test`	`19586` ns/op 12395 B/op 264 allocs/op	`17129` ns/op 12395 B/op 264 allocs/op	`1.14`
`BenchmarkDocument/nested_update_test - ns/op`	`19586` ns/op	`17129` ns/op	`1.14`
`BenchmarkDocument/nested_update_test - B/op`	`12395` B/op	`12395` B/op	`1`
`BenchmarkDocument/nested_update_test - allocs/op`	`264` allocs/op	`264` allocs/op	`1`
`BenchmarkDocument/delete_test`	`24057` ns/op 15923 B/op 347 allocs/op	`25918` ns/op 15923 B/op 347 allocs/op	`0.93`
`BenchmarkDocument/delete_test - ns/op`	`24057` ns/op	`25918` ns/op	`0.93`
`BenchmarkDocument/delete_test - B/op`	`15923` B/op	`15923` B/op	`1`
`BenchmarkDocument/delete_test - allocs/op`	`347` allocs/op	`347` allocs/op	`1`
`BenchmarkDocument/object_test`	`8924` ns/op 7073 B/op 122 allocs/op	`8806` ns/op 7073 B/op 122 allocs/op	`1.01`
`BenchmarkDocument/object_test - ns/op`	`8924` ns/op	`8806` ns/op	`1.01`
`BenchmarkDocument/object_test - B/op`	`7073` B/op	`7073` B/op	`1`
`BenchmarkDocument/object_test - allocs/op`	`122` allocs/op	`122` allocs/op	`1`
`BenchmarkDocument/array_test`	`30113` ns/op 12203 B/op 278 allocs/op	`30036` ns/op 12203 B/op 278 allocs/op	`1.00`
`BenchmarkDocument/array_test - ns/op`	`30113` ns/op	`30036` ns/op	`1.00`
`BenchmarkDocument/array_test - B/op`	`12203` B/op	`12203` B/op	`1`
`BenchmarkDocument/array_test - allocs/op`	`278` allocs/op	`278` allocs/op	`1`
`BenchmarkDocument/text_test`	`32146` ns/op 15323 B/op 492 allocs/op	`32282` ns/op 15323 B/op 492 allocs/op	`1.00`
`BenchmarkDocument/text_test - ns/op`	`32146` ns/op	`32282` ns/op	`1.00`
`BenchmarkDocument/text_test - B/op`	`15323` B/op	`15323` B/op	`1`
`BenchmarkDocument/text_test - allocs/op`	`492` allocs/op	`492` allocs/op	`1`
`BenchmarkDocument/text_composition_test`	`30479` ns/op 18718 B/op 504 allocs/op	`30559` ns/op 18718 B/op 504 allocs/op	`1.00`
`BenchmarkDocument/text_composition_test - ns/op`	`30479` ns/op	`30559` ns/op	`1.00`
`BenchmarkDocument/text_composition_test - B/op`	`18718` B/op	`18718` B/op	`1`
`BenchmarkDocument/text_composition_test - allocs/op`	`504` allocs/op	`504` allocs/op	`1`
`BenchmarkDocument/rich_text_test`	`84145` ns/op 40180 B/op 1183 allocs/op	`84716` ns/op 40180 B/op 1183 allocs/op	`0.99`
`BenchmarkDocument/rich_text_test - ns/op`	`84145` ns/op	`84716` ns/op	`0.99`
`BenchmarkDocument/rich_text_test - B/op`	`40180` B/op	`40180` B/op	`1`
`BenchmarkDocument/rich_text_test - allocs/op`	`1183` allocs/op	`1183` allocs/op	`1`
`BenchmarkDocument/counter_test`	`18645` ns/op 11874 B/op 258 allocs/op	`18570` ns/op 11875 B/op 258 allocs/op	`1.00`
`BenchmarkDocument/counter_test - ns/op`	`18645` ns/op	`18570` ns/op	`1.00`
`BenchmarkDocument/counter_test - B/op`	`11874` B/op	`11875` B/op	`1.00`
`BenchmarkDocument/counter_test - allocs/op`	`258` allocs/op	`258` allocs/op	`1`
`BenchmarkDocument/text_edit_gc_100`	`1319048` ns/op 872589 B/op 17282 allocs/op	`1322038` ns/op 872562 B/op 17282 allocs/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - ns/op`	`1319048` ns/op	`1322038` ns/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - B/op`	`872589` B/op	`872562` B/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - allocs/op`	`17282` allocs/op	`17282` allocs/op	`1`
`BenchmarkDocument/text_edit_gc_1000`	`50858460` ns/op 50547356 B/op 186744 allocs/op	`50761369` ns/op 50546440 B/op 186741 allocs/op	`1.00`
`BenchmarkDocument/text_edit_gc_1000 - ns/op`	`50858460` ns/op	`50761369` ns/op	`1.00`
`BenchmarkDocument/text_edit_gc_1000 - B/op`	`50547356` B/op	`50546440` B/op	`1.00`
`BenchmarkDocument/text_edit_gc_1000 - allocs/op`	`186744` allocs/op	`186741` allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_100`	`1936904` ns/op 1589046 B/op 15950 allocs/op	`1927409` ns/op 1589031 B/op 15951 allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_100 - ns/op`	`1936904` ns/op	`1927409` ns/op	`1.00`
`BenchmarkDocument/text_split_gc_100 - B/op`	`1589046` B/op	`1589031` B/op	`1.00`
`BenchmarkDocument/text_split_gc_100 - allocs/op`	`15950` allocs/op	`15951` allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_1000`	`116748116` ns/op 141482226 B/op 186149 allocs/op	`116138414` ns/op 141483012 B/op 186149 allocs/op	`1.01`
`BenchmarkDocument/text_split_gc_1000 - ns/op`	`116748116` ns/op	`116138414` ns/op	`1.01`
`BenchmarkDocument/text_split_gc_1000 - B/op`	`141482226` B/op	`141483012` B/op	`1.00`
`BenchmarkDocument/text_split_gc_1000 - allocs/op`	`186149` allocs/op	`186149` allocs/op	`1`
`BenchmarkDocument/text_delete_all_10000`	`17373963` ns/op 10214240 B/op 55687 allocs/op	`15920627` ns/op 10214378 B/op 55687 allocs/op	`1.09`
`BenchmarkDocument/text_delete_all_10000 - ns/op`	`17373963` ns/op	`15920627` ns/op	`1.09`
`BenchmarkDocument/text_delete_all_10000 - B/op`	`10214240` B/op	`10214378` B/op	`1.00`
`BenchmarkDocument/text_delete_all_10000 - allocs/op`	`55687` allocs/op	`55687` allocs/op	`1`
`BenchmarkDocument/text_delete_all_100000`	`311503662` ns/op 142958796 B/op 561666 allocs/op	`286191957` ns/op 142994620 B/op 561750 allocs/op	`1.09`
`BenchmarkDocument/text_delete_all_100000 - ns/op`	`311503662` ns/op	`286191957` ns/op	`1.09`
`BenchmarkDocument/text_delete_all_100000 - B/op`	`142958796` B/op	`142994620` B/op	`1.00`
`BenchmarkDocument/text_delete_all_100000 - allocs/op`	`561666` allocs/op	`561750` allocs/op	`1.00`
`BenchmarkDocument/text_100`	`230712` ns/op 120491 B/op 5182 allocs/op	`218714` ns/op 120489 B/op 5182 allocs/op	`1.05`
`BenchmarkDocument/text_100 - ns/op`	`230712` ns/op	`218714` ns/op	`1.05`
`BenchmarkDocument/text_100 - B/op`	`120491` B/op	`120489` B/op	`1.00`
`BenchmarkDocument/text_100 - allocs/op`	`5182` allocs/op	`5182` allocs/op	`1`
`BenchmarkDocument/text_1000`	`2450124` ns/op 1171296 B/op 51086 allocs/op	`2378530` ns/op 1171276 B/op 51086 allocs/op	`1.03`
`BenchmarkDocument/text_1000 - ns/op`	`2450124` ns/op	`2378530` ns/op	`1.03`
`BenchmarkDocument/text_1000 - B/op`	`1171296` B/op	`1171276` B/op	`1.00`
`BenchmarkDocument/text_1000 - allocs/op`	`51086` allocs/op	`51086` allocs/op	`1`
`BenchmarkDocument/array_1000`	`1229872` ns/op 1091633 B/op 11833 allocs/op	`1209815` ns/op 1091673 B/op 11834 allocs/op	`1.02`
`BenchmarkDocument/array_1000 - ns/op`	`1229872` ns/op	`1209815` ns/op	`1.02`
`BenchmarkDocument/array_1000 - B/op`	`1091633` B/op	`1091673` B/op	`1.00`
`BenchmarkDocument/array_1000 - allocs/op`	`11833` allocs/op	`11834` allocs/op	`1.00`
`BenchmarkDocument/array_10000`	`13744341` ns/op 9800480 B/op 120300 allocs/op	`13289936` ns/op 9800596 B/op 120299 allocs/op	`1.03`
`BenchmarkDocument/array_10000 - ns/op`	`13744341` ns/op	`13289936` ns/op	`1.03`
`BenchmarkDocument/array_10000 - B/op`	`9800480` B/op	`9800596` B/op	`1.00`
`BenchmarkDocument/array_10000 - allocs/op`	`120300` allocs/op	`120299` allocs/op	`1.00`
`BenchmarkDocument/array_gc_100`	`150533` ns/op 133287 B/op 1267 allocs/op	`146508` ns/op 133289 B/op 1267 allocs/op	`1.03`
`BenchmarkDocument/array_gc_100 - ns/op`	`150533` ns/op	`146508` ns/op	`1.03`
`BenchmarkDocument/array_gc_100 - B/op`	`133287` B/op	`133289` B/op	`1.00`
`BenchmarkDocument/array_gc_100 - allocs/op`	`1267` allocs/op	`1267` allocs/op	`1`
`BenchmarkDocument/array_gc_1000`	`1417159` ns/op 1159642 B/op 12882 allocs/op	`1410555` ns/op 1159867 B/op 12883 allocs/op	`1.00`
`BenchmarkDocument/array_gc_1000 - ns/op`	`1417159` ns/op	`1410555` ns/op	`1.00`
`BenchmarkDocument/array_gc_1000 - B/op`	`1159642` B/op	`1159867` B/op	`1.00`
`BenchmarkDocument/array_gc_1000 - allocs/op`	`12882` allocs/op	`12883` allocs/op	`1.00`
`BenchmarkDocument/counter_1000`	`205303` ns/op 193335 B/op 5773 allocs/op	`199922` ns/op 193335 B/op 5773 allocs/op	`1.03`
`BenchmarkDocument/counter_1000 - ns/op`	`205303` ns/op	`199922` ns/op	`1.03`
`BenchmarkDocument/counter_1000 - B/op`	`193335` B/op	`193335` B/op	`1`
`BenchmarkDocument/counter_1000 - allocs/op`	`5773` allocs/op	`5773` allocs/op	`1`
`BenchmarkDocument/counter_10000`	`2232469` ns/op 2088268 B/op 59780 allocs/op	`2161503` ns/op 2088251 B/op 59780 allocs/op	`1.03`
`BenchmarkDocument/counter_10000 - ns/op`	`2232469` ns/op	`2161503` ns/op	`1.03`
`BenchmarkDocument/counter_10000 - B/op`	`2088268` B/op	`2088251` B/op	`1.00`
`BenchmarkDocument/counter_10000 - allocs/op`	`59780` allocs/op	`59780` allocs/op	`1`
`BenchmarkDocument/object_1000`	`1397945` ns/op 1428289 B/op 9851 allocs/op	`1370491` ns/op 1428241 B/op 9850 allocs/op	`1.02`
`BenchmarkDocument/object_1000 - ns/op`	`1397945` ns/op	`1370491` ns/op	`1.02`
`BenchmarkDocument/object_1000 - B/op`	`1428289` B/op	`1428241` B/op	`1.00`
`BenchmarkDocument/object_1000 - allocs/op`	`9851` allocs/op	`9850` allocs/op	`1.00`
`BenchmarkDocument/object_10000`	`15642084` ns/op 12167154 B/op 100569 allocs/op	`15254780` ns/op 12168513 B/op 100572 allocs/op	`1.03`
`BenchmarkDocument/object_10000 - ns/op`	`15642084` ns/op	`15254780` ns/op	`1.03`
`BenchmarkDocument/object_10000 - B/op`	`12167154` B/op	`12168513` B/op	`1.00`
`BenchmarkDocument/object_10000 - allocs/op`	`100569` allocs/op	`100572` allocs/op	`1.00`
`BenchmarkDocument/tree_100`	`1036621` ns/op 943957 B/op 6103 allocs/op	`1020342` ns/op 943956 B/op 6103 allocs/op	`1.02`
`BenchmarkDocument/tree_100 - ns/op`	`1036621` ns/op	`1020342` ns/op	`1.02`
`BenchmarkDocument/tree_100 - B/op`	`943957` B/op	`943956` B/op	`1.00`
`BenchmarkDocument/tree_100 - allocs/op`	`6103` allocs/op	`6103` allocs/op	`1`
`BenchmarkDocument/tree_1000`	`74462915` ns/op 86460531 B/op 60116 allocs/op	`72748940` ns/op 86460446 B/op 60116 allocs/op	`1.02`
`BenchmarkDocument/tree_1000 - ns/op`	`74462915` ns/op	`72748940` ns/op	`1.02`
`BenchmarkDocument/tree_1000 - B/op`	`86460531` B/op	`86460446` B/op	`1.00`
`BenchmarkDocument/tree_1000 - allocs/op`	`60116` allocs/op	`60116` allocs/op	`1`
`BenchmarkDocument/tree_10000`	`9549884515` ns/op 8580673616 B/op 600256 allocs/op	`9344816675` ns/op 8580671488 B/op 600224 allocs/op	`1.02`
`BenchmarkDocument/tree_10000 - ns/op`	`9549884515` ns/op	`9344816675` ns/op	`1.02`
`BenchmarkDocument/tree_10000 - B/op`	`8580673616` B/op	`8580671488` B/op	`1.00`
`BenchmarkDocument/tree_10000 - allocs/op`	`600256` allocs/op	`600224` allocs/op	`1.00`
`BenchmarkDocument/tree_delete_all_1000`	`78865098` ns/op 87533780 B/op 75273 allocs/op	`73752345` ns/op 87509262 B/op 75268 allocs/op	`1.07`
`BenchmarkDocument/tree_delete_all_1000 - ns/op`	`78865098` ns/op	`73752345` ns/op	`1.07`
`BenchmarkDocument/tree_delete_all_1000 - B/op`	`87533780` B/op	`87509262` B/op	`1.00`
`BenchmarkDocument/tree_delete_all_1000 - allocs/op`	`75273` allocs/op	`75268` allocs/op	`1.00`
`BenchmarkDocument/tree_edit_gc_100`	`4020570` ns/op 4147246 B/op 15147 allocs/op	`3751719` ns/op 4148278 B/op 15147 allocs/op	`1.07`
`BenchmarkDocument/tree_edit_gc_100 - ns/op`	`4020570` ns/op	`3751719` ns/op	`1.07`
`BenchmarkDocument/tree_edit_gc_100 - B/op`	`4147246` B/op	`4148278` B/op	`1.00`
`BenchmarkDocument/tree_edit_gc_100 - allocs/op`	`15147` allocs/op	`15147` allocs/op	`1`
`BenchmarkDocument/tree_edit_gc_1000`	`320962629` ns/op 383748666 B/op 154872 allocs/op	`293866389` ns/op 383744620 B/op 154850 allocs/op	`1.09`
`BenchmarkDocument/tree_edit_gc_1000 - ns/op`	`320962629` ns/op	`293866389` ns/op	`1.09`
`BenchmarkDocument/tree_edit_gc_1000 - B/op`	`383748666` B/op	`383744620` B/op	`1.00`
`BenchmarkDocument/tree_edit_gc_1000 - allocs/op`	`154872` allocs/op	`154850` allocs/op	`1.00`
`BenchmarkDocument/tree_split_gc_100`	`2736426` ns/op 2413127 B/op 11131 allocs/op	`2480875` ns/op 2412997 B/op 11131 allocs/op	`1.10`
`BenchmarkDocument/tree_split_gc_100 - ns/op`	`2736426` ns/op	`2480875` ns/op	`1.10`
`BenchmarkDocument/tree_split_gc_100 - B/op`	`2413127` B/op	`2412997` B/op	`1.00`
`BenchmarkDocument/tree_split_gc_100 - allocs/op`	`11131` allocs/op	`11131` allocs/op	`1`
`BenchmarkDocument/tree_split_gc_1000`	`197579593` ns/op 222251724 B/op 122009 allocs/op	`177728909` ns/op 222252022 B/op 121998 allocs/op	`1.11`
`BenchmarkDocument/tree_split_gc_1000 - ns/op`	`197579593` ns/op	`177728909` ns/op	`1.11`
`BenchmarkDocument/tree_split_gc_1000 - B/op`	`222251724` B/op	`222252022` B/op	`1.00`
`BenchmarkDocument/tree_split_gc_1000 - allocs/op`	`122009` allocs/op	`121998` allocs/op	`1.00`
`BenchmarkRPC/client_to_server`	`421534841` ns/op 19708426 B/op 223540 allocs/op	`417618189` ns/op 20917842 B/op 228433 allocs/op	`1.01`
`BenchmarkRPC/client_to_server - ns/op`	`421534841` ns/op	`417618189` ns/op	`1.01`
`BenchmarkRPC/client_to_server - B/op`	`19708426` B/op	`20917842` B/op	`0.94`
`BenchmarkRPC/client_to_server - allocs/op`	`223540` allocs/op	`228433` allocs/op	`0.98`
`BenchmarkRPC/client_to_client_via_server`	`781048370` ns/op 42590964 B/op 468268 allocs/op	`779122100` ns/op 42257820 B/op 476664 allocs/op	`1.00`
`BenchmarkRPC/client_to_client_via_server - ns/op`	`781048370` ns/op	`779122100` ns/op	`1.00`
`BenchmarkRPC/client_to_client_via_server - B/op`	`42590964` B/op	`42257820` B/op	`1.01`
`BenchmarkRPC/client_to_client_via_server - allocs/op`	`468268` allocs/op	`476664` allocs/op	`0.98`
`BenchmarkRPC/attach_large_document`	`1519763733` ns/op 1897393928 B/op 12552 allocs/op	`1259370310` ns/op 1932711680 B/op 13568 allocs/op	`1.21`
`BenchmarkRPC/attach_large_document - ns/op`	`1519763733` ns/op	`1259370310` ns/op	`1.21`
`BenchmarkRPC/attach_large_document - B/op`	`1897393928` B/op	`1932711680` B/op	`0.98`
`BenchmarkRPC/attach_large_document - allocs/op`	`12552` allocs/op	`13568` allocs/op	`0.93`
`BenchmarkRPC/adminCli_to_server`	`535788044` ns/op 35998404 B/op 289701 allocs/op	`526685870` ns/op 35957480 B/op 289555 allocs/op	`1.02`
`BenchmarkRPC/adminCli_to_server - ns/op`	`535788044` ns/op	`526685870` ns/op	`1.02`
`BenchmarkRPC/adminCli_to_server - B/op`	`35998404` B/op	`35957480` B/op	`1.00`
`BenchmarkRPC/adminCli_to_server - allocs/op`	`289701` allocs/op	`289555` allocs/op	`1.00`
`BenchmarkLocker`	`66.62` ns/op 16 B/op 1 allocs/op	`64.28` ns/op 16 B/op 1 allocs/op	`1.04`
`BenchmarkLocker - ns/op`	`66.62` ns/op	`64.28` ns/op	`1.04`
`BenchmarkLocker - B/op`	`16` B/op	`16` B/op	`1`
`BenchmarkLocker - allocs/op`	`1` allocs/op	`1` allocs/op	`1`
`BenchmarkLockerParallel`	`39.79` ns/op 0 B/op 0 allocs/op	`38.39` ns/op 0 B/op 0 allocs/op	`1.04`
`BenchmarkLockerParallel - ns/op`	`39.79` ns/op	`38.39` ns/op	`1.04`
`BenchmarkLockerParallel - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkLockerParallel - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkLockerMoreKeys`	`153.9` ns/op 15 B/op 0 allocs/op	`145.9` ns/op 15 B/op 0 allocs/op	`1.05`
`BenchmarkLockerMoreKeys - ns/op`	`153.9` ns/op	`145.9` ns/op	`1.05`
`BenchmarkLockerMoreKeys - B/op`	`15` B/op	`15` B/op	`1`
`BenchmarkLockerMoreKeys - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkChange/Push_10_Changes`	`4473899` ns/op 144473 B/op 1571 allocs/op	`4368453` ns/op 143556 B/op 1571 allocs/op	`1.02`
`BenchmarkChange/Push_10_Changes - ns/op`	`4473899` ns/op	`4368453` ns/op	`1.02`
`BenchmarkChange/Push_10_Changes - B/op`	`144473` B/op	`143556` B/op	`1.01`
`BenchmarkChange/Push_10_Changes - allocs/op`	`1571` allocs/op	`1571` allocs/op	`1`
`BenchmarkChange/Push_100_Changes`	`16487489` ns/op 730054 B/op 8191 allocs/op	`16247890` ns/op 697742 B/op 8185 allocs/op	`1.01`
`BenchmarkChange/Push_100_Changes - ns/op`	`16487489` ns/op	`16247890` ns/op	`1.01`
`BenchmarkChange/Push_100_Changes - B/op`	`730054` B/op	`697742` B/op	`1.05`
`BenchmarkChange/Push_100_Changes - allocs/op`	`8191` allocs/op	`8185` allocs/op	`1.00`
`BenchmarkChange/Push_1000_Changes`	`132140125` ns/op 7113647 B/op 77175 allocs/op	`131794613` ns/op 6824378 B/op 77157 allocs/op	`1.00`
`BenchmarkChange/Push_1000_Changes - ns/op`	`132140125` ns/op	`131794613` ns/op	`1.00`
`BenchmarkChange/Push_1000_Changes - B/op`	`7113647` B/op	`6824378` B/op	`1.04`
`BenchmarkChange/Push_1000_Changes - allocs/op`	`77175` allocs/op	`77157` allocs/op	`1.00`
`BenchmarkChange/Pull_10_Changes`	`3693351` ns/op 123682 B/op 1403 allocs/op	`3588130` ns/op 123990 B/op 1403 allocs/op	`1.03`
`BenchmarkChange/Pull_10_Changes - ns/op`	`3693351` ns/op	`3588130` ns/op	`1.03`
`BenchmarkChange/Pull_10_Changes - B/op`	`123682` B/op	`123990` B/op	`1.00`
`BenchmarkChange/Pull_10_Changes - allocs/op`	`1403` allocs/op	`1403` allocs/op	`1`
`BenchmarkChange/Pull_100_Changes`	`5277847` ns/op 350937 B/op 4949 allocs/op	`5098234` ns/op 351524 B/op 4947 allocs/op	`1.04`
`BenchmarkChange/Pull_100_Changes - ns/op`	`5277847` ns/op	`5098234` ns/op	`1.04`
`BenchmarkChange/Pull_100_Changes - B/op`	`350937` B/op	`351524` B/op	`1.00`
`BenchmarkChange/Pull_100_Changes - allocs/op`	`4949` allocs/op	`4947` allocs/op	`1.00`
`BenchmarkChange/Pull_1000_Changes`	`10866418` ns/op 2221370 B/op 42676 allocs/op	`10205605` ns/op 2221031 B/op 42668 allocs/op	`1.06`
`BenchmarkChange/Pull_1000_Changes - ns/op`	`10866418` ns/op	`10205605` ns/op	`1.06`
`BenchmarkChange/Pull_1000_Changes - B/op`	`2221370` B/op	`2221031` B/op	`1.00`
`BenchmarkChange/Pull_1000_Changes - allocs/op`	`42676` allocs/op	`42668` allocs/op	`1.00`
`BenchmarkSnapshot/Push_3KB_snapshot`	`18836255` ns/op 855791 B/op 8195 allocs/op	`18356423` ns/op 813586 B/op 8187 allocs/op	`1.03`
`BenchmarkSnapshot/Push_3KB_snapshot - ns/op`	`18836255` ns/op	`18356423` ns/op	`1.03`
`BenchmarkSnapshot/Push_3KB_snapshot - B/op`	`855791` B/op	`813586` B/op	`1.05`
`BenchmarkSnapshot/Push_3KB_snapshot - allocs/op`	`8195` allocs/op	`8187` allocs/op	`1.00`
`BenchmarkSnapshot/Push_30KB_snapshot`	`135148126` ns/op 7864338 B/op 82537 allocs/op	`134315109` ns/op 7250191 B/op 78589 allocs/op	`1.01`
`BenchmarkSnapshot/Push_30KB_snapshot - ns/op`	`135148126` ns/op	`134315109` ns/op	`1.01`
`BenchmarkSnapshot/Push_30KB_snapshot - B/op`	`7864338` B/op	`7250191` B/op	`1.08`
`BenchmarkSnapshot/Push_30KB_snapshot - allocs/op`	`82537` allocs/op	`78589` allocs/op	`1.05`
`BenchmarkSnapshot/Pull_3KB_snapshot`	`7592061` ns/op 1139822 B/op 19419 allocs/op	`7094386` ns/op 1139095 B/op 19417 allocs/op	`1.07`
`BenchmarkSnapshot/Pull_3KB_snapshot - ns/op`	`7592061` ns/op	`7094386` ns/op	`1.07`
`BenchmarkSnapshot/Pull_3KB_snapshot - B/op`	`1139822` B/op	`1139095` B/op	`1.00`
`BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op`	`19419` allocs/op	`19417` allocs/op	`1.00`
`BenchmarkSnapshot/Pull_30KB_snapshot`	`19713404` ns/op 9326137 B/op 187583 allocs/op	`17461374` ns/op 9301176 B/op 187556 allocs/op	`1.13`
`BenchmarkSnapshot/Pull_30KB_snapshot - ns/op`	`19713404` ns/op	`17461374` ns/op	`1.13`
`BenchmarkSnapshot/Pull_30KB_snapshot - B/op`	`9326137` B/op	`9301176` B/op	`1.00`
`BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op`	`187583` allocs/op	`187556` allocs/op	`1.00`
`BenchmarkSplayTree/stress_test_100000`	`0.2024` ns/op 0 B/op 0 allocs/op	`0.2001` ns/op 0 B/op 0 allocs/op	`1.01`
`BenchmarkSplayTree/stress_test_100000 - ns/op`	`0.2024` ns/op	`0.2001` ns/op	`1.01`
`BenchmarkSplayTree/stress_test_100000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/stress_test_100000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/stress_test_200000`	`0.3977` ns/op 0 B/op 0 allocs/op	`0.391` ns/op 0 B/op 0 allocs/op	`1.02`
`BenchmarkSplayTree/stress_test_200000 - ns/op`	`0.3977` ns/op	`0.391` ns/op	`1.02`
`BenchmarkSplayTree/stress_test_200000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/stress_test_200000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/stress_test_300000`	`0.6187` ns/op 0 B/op 0 allocs/op	`0.5963` ns/op 0 B/op 0 allocs/op	`1.04`
`BenchmarkSplayTree/stress_test_300000 - ns/op`	`0.6187` ns/op	`0.5963` ns/op	`1.04`
`BenchmarkSplayTree/stress_test_300000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/stress_test_300000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/random_access_100000`	`0.01306` ns/op 0 B/op 0 allocs/op	`0.01344` ns/op 0 B/op 0 allocs/op	`0.97`
`BenchmarkSplayTree/random_access_100000 - ns/op`	`0.01306` ns/op	`0.01344` ns/op	`0.97`
`BenchmarkSplayTree/random_access_100000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/random_access_100000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/random_access_200000`	`0.03384` ns/op 0 B/op 0 allocs/op	`0.02425` ns/op 0 B/op 0 allocs/op	`1.40`
`BenchmarkSplayTree/random_access_200000 - ns/op`	`0.03384` ns/op	`0.02425` ns/op	`1.40`
`BenchmarkSplayTree/random_access_200000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/random_access_200000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/random_access_300000`	`0.045` ns/op 0 B/op 0 allocs/op	`0.03793` ns/op 0 B/op 0 allocs/op	`1.19`
`BenchmarkSplayTree/random_access_300000 - ns/op`	`0.045` ns/op	`0.03793` ns/op	`1.19`
`BenchmarkSplayTree/random_access_300000 - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/random_access_300000 - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSplayTree/editing_trace_bench`	`0.00226` ns/op 0 B/op 0 allocs/op	`0.002062` ns/op 0 B/op 0 allocs/op	`1.10`
`BenchmarkSplayTree/editing_trace_bench - ns/op`	`0.00226` ns/op	`0.002062` ns/op	`1.10`
`BenchmarkSplayTree/editing_trace_bench - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkSplayTree/editing_trace_bench - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkSync/memory_sync_10_test`	`8620` ns/op 3765 B/op 69 allocs/op	`8123` ns/op 3765 B/op 69 allocs/op	`1.06`
`BenchmarkSync/memory_sync_10_test - ns/op`	`8620` ns/op	`8123` ns/op	`1.06`
`BenchmarkSync/memory_sync_10_test - B/op`	`3765` B/op	`3765` B/op	`1`
`BenchmarkSync/memory_sync_10_test - allocs/op`	`69` allocs/op	`69` allocs/op	`1`
`BenchmarkSync/memory_sync_100_test`	`54675` ns/op 11114 B/op 303 allocs/op	`53247` ns/op 11111 B/op 303 allocs/op	`1.03`
`BenchmarkSync/memory_sync_100_test - ns/op`	`54675` ns/op	`53247` ns/op	`1.03`
`BenchmarkSync/memory_sync_100_test - B/op`	`11114` B/op	`11111` B/op	`1.00`
`BenchmarkSync/memory_sync_100_test - allocs/op`	`303` allocs/op	`303` allocs/op	`1`
`BenchmarkSync/memory_sync_1000_test`	`589260` ns/op 77150 B/op 2160 allocs/op	`553195` ns/op 78668 B/op 2265 allocs/op	`1.07`
`BenchmarkSync/memory_sync_1000_test - ns/op`	`589260` ns/op	`553195` ns/op	`1.07`
`BenchmarkSync/memory_sync_1000_test - B/op`	`77150` B/op	`78668` B/op	`0.98`
`BenchmarkSync/memory_sync_1000_test - allocs/op`	`2160` allocs/op	`2265` allocs/op	`0.95`
`BenchmarkSync/memory_sync_10000_test`	`7357624` ns/op 759731 B/op 20523 allocs/op	`7480887` ns/op 737765 B/op 20242 allocs/op	`0.98`
`BenchmarkSync/memory_sync_10000_test - ns/op`	`7357624` ns/op	`7480887` ns/op	`0.98`
`BenchmarkSync/memory_sync_10000_test - B/op`	`759731` B/op	`737765` B/op	`1.03`
`BenchmarkSync/memory_sync_10000_test - allocs/op`	`20523` allocs/op	`20242` allocs/op	`1.01`
`BenchmarkTextEditing`	`5002713634` ns/op 3982603008 B/op 20647701 allocs/op	`4853340799` ns/op 3982622464 B/op 20647736 allocs/op	`1.03`
`BenchmarkTextEditing - ns/op`	`5002713634` ns/op	`4853340799` ns/op	`1.03`
`BenchmarkTextEditing - B/op`	`3982603008` B/op	`3982622464` B/op	`1.00`
`BenchmarkTextEditing - allocs/op`	`20647701` allocs/op	`20647736` allocs/op	`1.00`
`BenchmarkTree/10000_vertices_to_protobuf`	`4381754` ns/op 6262985 B/op 70025 allocs/op	`3527100` ns/op 6262996 B/op 70025 allocs/op	`1.24`
`BenchmarkTree/10000_vertices_to_protobuf - ns/op`	`4381754` ns/op	`3527100` ns/op	`1.24`
`BenchmarkTree/10000_vertices_to_protobuf - B/op`	`6262985` B/op	`6262996` B/op	`1.00`
`BenchmarkTree/10000_vertices_to_protobuf - allocs/op`	`70025` allocs/op	`70025` allocs/op	`1`
`BenchmarkTree/10000_vertices_from_protobuf`	`219913186` ns/op 442170248 B/op 290039 allocs/op	`159944457` ns/op 442173854 B/op 290051 allocs/op	`1.37`
`BenchmarkTree/10000_vertices_from_protobuf - ns/op`	`219913186` ns/op	`159944457` ns/op	`1.37`
`BenchmarkTree/10000_vertices_from_protobuf - B/op`	`442170248` B/op	`442173854` B/op	`1.00`
`BenchmarkTree/10000_vertices_from_protobuf - allocs/op`	`290039` allocs/op	`290051` allocs/op	`1.00`
`BenchmarkTree/20000_vertices_to_protobuf`	`9015350` ns/op 12716875 B/op 140028 allocs/op	`7745215` ns/op 12716921 B/op 140028 allocs/op	`1.16`
`BenchmarkTree/20000_vertices_to_protobuf - ns/op`	`9015350` ns/op	`7745215` ns/op	`1.16`
`BenchmarkTree/20000_vertices_to_protobuf - B/op`	`12716875` B/op	`12716921` B/op	`1.00`
`BenchmarkTree/20000_vertices_to_protobuf - allocs/op`	`140028` allocs/op	`140028` allocs/op	`1`
`BenchmarkTree/20000_vertices_from_protobuf`	`874705434` ns/op 1697272664 B/op 580091 allocs/op	`698654276` ns/op 1697276888 B/op 580091 allocs/op	`1.25`
`BenchmarkTree/20000_vertices_from_protobuf - ns/op`	`874705434` ns/op	`698654276` ns/op	`1.25`
`BenchmarkTree/20000_vertices_from_protobuf - B/op`	`1697272664` B/op	`1697276888` B/op	`1.00`
`BenchmarkTree/20000_vertices_from_protobuf - allocs/op`	`580091` allocs/op	`580091` allocs/op	`1`
`BenchmarkTree/30000_vertices_to_protobuf`	`13663306` ns/op 19318337 B/op 210030 allocs/op	`12444412` ns/op 19318248 B/op 210030 allocs/op	`1.10`
`BenchmarkTree/30000_vertices_to_protobuf - ns/op`	`13663306` ns/op	`12444412` ns/op	`1.10`
`BenchmarkTree/30000_vertices_to_protobuf - B/op`	`19318337` B/op	`19318248` B/op	`1.00`
`BenchmarkTree/30000_vertices_to_protobuf - allocs/op`	`210030` allocs/op	`210030` allocs/op	`1`
`BenchmarkTree/30000_vertices_from_protobuf`	`1982640715` ns/op 3752056480 B/op 870091 allocs/op	`1614693599` ns/op 3752052856 B/op 870052 allocs/op	`1.23`
`BenchmarkTree/30000_vertices_from_protobuf - ns/op`	`1982640715` ns/op	`1614693599` ns/op	`1.23`
`BenchmarkTree/30000_vertices_from_protobuf - B/op`	`3752056480` B/op	`3752052856` B/op	`1.00`
`BenchmarkTree/30000_vertices_from_protobuf - allocs/op`	`870091` allocs/op	`870052` allocs/op	`1.00`

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

…detach

…ed query efficiency

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)

server/backend/database/mongo/client_test.go (1)
83-85: Consider adding performance benchmarks.

Given that this PR aims to improve performance from 150ms to 30ms for document detachment operations, it would be valuable to add performance benchmarks to validate and track these improvements.

Example benchmark structure:
func BenchmarkFindLatestChangeInfo(b *testing.B) {
    cli := setupTestWithDummyData(b)
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        testcases.RunFindLatestChangeInfoTest(b, cli, dummyProjectID)
    }
}
server/backend/database/mongo/indexes.go (1)
130-137: Consider documenting the index strategy.

Since this index plays a crucial role in the performance optimization (reducing execution time from 150ms to 30ms), consider adding a comment explaining:

The purpose of this index

Why both indexes are needed

The performance implications

Example comment:
 		}, {
+			// Index optimized for FindLatestChangeInfoByActor queries
+			// Reduces document detach operation time from ~150ms to ~30ms
+			// Maintains sharding compatibility with doc_id prefix
 			Keys: bsonx.Doc{
server/backend/database/database.go (1)
222-229: Enhance method documentation for better clarity.

While the basic purpose is documented, consider adding more details about:

The relationship between serverSeq and the returned change

Whether the returned change must have a sequence number less than or equal to the provided serverSeq

Error conditions that could trigger the error return

Consider updating the documentation to:
-// FindLatestChangeInfoByActor returns the latest change created by given actorID.
+// FindLatestChangeInfoByActor returns the latest change created by the given actorID
+// with a server sequence number less than or equal to the provided serverSeq.
+// Returns (nil, ErrChangeNotFound) if no matching change exists.
server/backend/database/testcases/testcases.go (1)
526-594: LGTM! Well-structured test implementation.

The test function is well-implemented with clear setup, execution, and verification phases. It effectively validates the FindLatestChangeInfoByActor functionality by:

Creating and setting up test data

Making changes to verify the latest change info

Comparing results between different methods of retrieving changes

However, here are some suggestions to make the test more robust:

Add test cases for edge cases (empty changes, multiple actors)

Consider testing error scenarios (invalid actor ID, non-existent document)

Here's a suggested addition to test error scenarios:
 	t.Run("store changes and find latest changeInfo test", func(t *testing.T) {
+		// ... existing test code ...
+	})
+
+	t.Run("find latest changeInfo error scenarios test", func(t *testing.T) {
+		ctx := context.Background()
+		
+		// Test with non-existent document
+		_, err := db.FindLatestChangeInfoByActor(
+			ctx,
+			types.DocRefKey{ProjectID: projectID, DocID: "non-existent"},
+			types.ID("invalid-actor"),
+			10,
+		)
+		assert.Error(t, err)
+		
+		// Test with invalid actor ID
+		docKey := key.Key(fmt.Sprintf("tests$%s", t.Name()))
+		clientInfo, _ := db.ActivateClient(ctx, projectID, t.Name())
+		docInfo, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), docKey, true)
+		
+		_, err = db.FindLatestChangeInfoByActor(
+			ctx,
+			docInfo.RefKey(),
+			types.ID("invalid-actor"),
+			10,
+		)
+		assert.Error(t, err)
+	})

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4485a28 and 0a770cd.

📒 Files selected for processing (9)

server/backend/database/database.go (1 hunks)
server/backend/database/memory/database.go (1 hunks)
server/backend/database/memory/database_test.go (1 hunks)
server/backend/database/memory/indexes.go (1 hunks)
server/backend/database/mongo/client.go (1 hunks)
server/backend/database/mongo/client_test.go (1 hunks)
server/backend/database/mongo/indexes.go (1 hunks)
server/backend/database/testcases/testcases.go (1 hunks)
server/rpc/cluster_server.go (3 hunks)

🔇 Additional comments (14)

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

67-69: LGTM! The test case is well-positioned and follows the established patterns.

The new test case for FindLatestChangeInfoByActor functionality:

Maintains consistency with other test cases
Uses the same project ID constant
Is logically placed among related change info tests

Let's verify the test implementation in the testcases package:

✅ Verification successful

Test implementation for FindLatestChangeInfoByActor is complete and well-structured

The test case correctly validates the functionality by:

Setting up test data with multiple changes from a specific actor
Creating and storing changes with proper server sequences
Verifying the retrieval of the latest change for the actor
Testing across both memory and mongo database implementations

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of RunFindLatestChangeInfoTest
# Expected: Should find the test implementation that validates FindLatestChangeInfoByActor

# Search for the test implementation
rg -A 10 "RunFindLatestChangeInfoTest" --type go

Length of output: 3001

Script:

#!/bin/bash
# Let's examine the full implementation of RunFindLatestChangeInfoTest to verify its correctness
rg -A 50 "func RunFindLatestChangeInfoTest" --type go

Length of output: 4010

Script:

#!/bin/bash
# Let's see if there's more to the test implementation and check for FindLatestChangeInfoByActor usage
rg -A 30 "FindLatestChangeInfoByActor" --type go

Length of output: 13279

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

83-85: LGTM! Test case properly validates the new functionality.

The new test case is well-integrated into the existing test suite and follows the established patterns for testing MongoDB client functionality.

server/backend/database/mongo/indexes.go (2)

130-137: Verify index memory impact.

While the index design is optimal for query performance, having two similar compound indexes might impact memory usage. The existing index {doc_id, project_id, server_seq} might be partially redundant since MongoDB can use index prefixes.

Let's analyze the potential memory impact:

#!/bin/bash
# Description: Check for queries that specifically require the first index
# to determine if we can consolidate the indexes

# Search for queries using the first index pattern
rg -A 5 'Find.*server_seq.*project_id.*!actor_id'

# Search for any explicit index hints or explanations
rg -A 5 'hint.*changes.*index'

130-137: LGTM! The new compound index is well-designed for the optimization.

The new index aligns perfectly with the PR's objective of optimizing document detach operations. The compound index structure {doc_id: 1, project_id: 1, actor_id: 1, server_seq: 1} will efficiently support queries filtering by actor_id while maintaining the sharding strategy.

A few key points worth noting:

The index preserves the shard key (doc_id) as the prefix
The uniqueness constraint ensures data integrity
The index order supports both equality matches and range queries on server_seq

Let's verify the index usage for the new query pattern:

✅ Verification successful

Let me search for the implementation of FindLatestChangeInfoByActor to verify the index usage.

Let me try a different search pattern to find the implementation.

Let me search for the collection constants and index definitions.

Let me search specifically for the indexes.go file content.

The new compound index is well-designed and will be used efficiently

Looking at the implementation in indexes.go, the compound index on ColChanges collection with keys {doc_id: 1, project_id: 1, actor_id: 1, server_seq: 1} will be used effectively by FindLatestChangeInfoByActor() because:

The query matches the index prefix (doc_id, project_id, actor_id)
The sort on server_seq is covered by the index
The shard key (doc_id) is maintained as the prefix
The uniqueness constraint ensures data integrity

The MongoDB query:

FindOne(ctx, bson.M{
    "project_id": docRefKey.ProjectID,
    "doc_id": docRefKey.DocID, 
    "actor_id": actorID,
    "server_seq": bson.M{"$lte": serverSeq}
}, options.FindOne().SetSort(bson.M{"server_seq": -1}))

will efficiently use the index for both filtering and sorting.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the new FindLatestChangeInfoByActor method uses the new index
# Expected: The method should use the new compound index for efficient querying

# Search for the query implementation
ast-grep --pattern 'FindLatestChangeInfoByActor($$$) {
  $$$
}'

# Search for any MongoDB query patterns that might not utilize the index effectively
rg -A 5 'Find|Aggregate.*actor_id'

Length of output: 206358

Script:

#!/bin/bash
# Search for FindLatestChangeInfoByActor implementation
ast-grep --pattern 'FindLatestChangeInfoByActor($$$) {
  $$$
}'

# Search for any related MongoDB queries and index hints
rg -A 10 'FindLatestChangeInfoByActor.*actor_id.*server_seq'

Length of output: 128

Script:

#!/bin/bash
# Search for FindLatestChangeInfoByActor implementation in mongo client
rg -A 15 'func.*FindLatestChangeInfoByActor' server/backend/database/mongo/

# Search for the collection definition and indexes
rg -A 5 'const.*ColChanges.*=' server/backend/database/mongo/

Length of output: 1216

Script:

#!/bin/bash
# Search for collection constants
rg -A 5 'const \(' server/backend/database/mongo/

# Search for index creation in the mongo package
rg -A 10 'CreateIndexes' server/backend/database/mongo/

Length of output: 1454

Script:

#!/bin/bash
# Show the content of indexes.go file
cat server/backend/database/mongo/indexes.go

# Search for any index-related tests
rg -A 5 'TestCreateIndexes' server/backend/database/mongo/

Length of output: 5651

server/backend/database/memory/indexes.go (2)

170-179: LGTM! Well-structured compound index for optimizing actor-based queries.

The new compound index doc_id_actor_id_server_seq is well-designed for efficiently retrieving the latest changes by actor, which aligns with the PR's goal of optimizing document detach operations.

170-179: Verify if project_id should be included in the index.

The PR objectives mention changing the index structure to include project_id, but it's not included in this implementation. Please verify if this is intentional or if project_id should be added to the compound index.

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

222-229: LGTM! Method signature aligns with optimization goals.

The new method supports the PR's optimization goal by enabling direct retrieval of actor-specific changes, which helps reduce the document detach operation time from 150ms to 30ms.

Let's verify the method's integration with the existing codebase:

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

1015-1044: Implementation looks good with efficient index usage.

The implementation of FindLatestChangeInfoByActor is well-structured and efficient:

Uses ReverseLowerBound with a compound index for optimal performance
Properly handles transactions and error cases
Clear error messages with proper context

Let's verify the index definition:

✅ Verification successful

Let me gather more information about the compound index implementation to ensure it's properly defined for all required fields.

Index implementation verified and correctly supports the query pattern

The compound index doc_id_actor_id_server_seq is properly defined with all required fields in the correct order:

DocID (string) as the primary field
ActorID (string) as the secondary field
ServerSeq (integer) as the tertiary field

This structure perfectly aligns with the ReverseLowerBound query in FindLatestChangeInfoByActor, ensuring efficient lookups.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the compound index definition for the new method.
# Expect: Index definition for "doc_id_actor_id_server_seq" in the schema.

rg -A 3 'doc_id_actor_id_server_seq' --type go

Length of output: 717

Script:

#!/bin/bash
# Get the complete compound index definition
rg -B 2 -A 10 'doc_id_actor_id_server_seq' server/backend/database/memory/indexes.go

Length of output: 426

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

1000-1013: LGTM: Error handling follows best practices.

The error handling is well-implemented:

Returns empty struct instead of nil for no documents case
Properly wraps errors with context
Uses appropriate error messages

980-986: Implementation aligns with PR objectives.

The new method effectively supports the optimization of document detach operations by providing direct access to the latest change info by actor, contributing to the significant performance improvement mentioned in the PR objectives (reduction from 150ms to 30ms).

980-1013: Consider adding an index hint for query optimization.

The implementation looks good with proper error handling and empty result handling. However, for optimal query performance, consider adding an index hint since this query relies on multiple fields.

Let's verify if the required index exists:

If the index doesn't exist, add this compound index to improve query performance:

 if err := db.Collection(ColChanges).CreateIndexes(ctx, []mongo.IndexModel{
+  {
+    Keys: bson.D{
+      {Key: "project_id", Value: 1},
+      {Key: "doc_id", Value: 1},
+      {Key: "actor_id", Value: 1},
+      {Key: "server_seq", Value: -1},
+    },
+  },

server/rpc/cluster_server.go (3)

28-29: Import statements updated appropriately

The new imports for "github.com/yorkie-team/yorkie/pkg/document/change" and "github.com/yorkie-team/yorkie/pkg/document/innerpresence" are necessary for the optimized detachment logic.

126-128: Changing sync mode to SyncModePushOnly is appropriate

Modifying the synchronization mode to types.SyncModePushOnly aligns with the optimized detachment process and ensures that only necessary changes are pushed.

99-104: ⚠️ Potential issue

Correct actor ID conversion in FindLatestChangeInfoByActor

The function FindLatestChangeInfoByActor expects an actorID of type types.ID. Currently, types.ID(req.Msg.ClientId) may not correctly convert the client ID string to the required type. To ensure proper conversion and consistency, use types.IDFromActorID(actorID) instead.

Apply this fix:

-        types.ID(req.Msg.ClientId),
+        types.IDFromActorID(actorID),

Likely invalid or redundant comment.

hackerwins

Thanks for your contribution.

Make change pack directly when document detach

a8605ef

raararaara changed the title ~~Improve ChangePack creation logic for p.Clear() operation~~ Improve ChangePack creation logic for p.Clear() when document detach Oct 31, 2024

Add latest change info when creating change pack

5aacd18

github-actions bot reviewed Oct 31, 2024

View reviewed changes

raararaara added 5 commits November 4, 2024 22:16

Fix lamport and version vector when detach

6e69b63

Apply latest change info when generating change during clusterServer.…

267b9fc

…detach

Add tests for FindLatestChangeInfoByActor

f1baf05

Remove unnecessary lamport adjustment logic

9f93a91

Add actor_id field to Change collection index in MongoDB for improv…

e56e9f8

…ed query efficiency

raararaara changed the title ~~Improve ChangePack creation logic for p.Clear() when document detach~~ Optimize document detach operation via Cluster Server Nov 8, 2024

Revise change collection index

0a770cd

raararaara marked this pull request as ready for review November 8, 2024 08:10

coderabbitai bot reviewed Nov 8, 2024

View reviewed changes

raararaara requested a review from hackerwins November 8, 2024 08:34

Clean up codes

c3f9542

hackerwins force-pushed the deactivate-optimize branch from a4f2253 to c3f9542 Compare November 8, 2024 10:00

hackerwins approved these changes Nov 8, 2024

View reviewed changes

hackerwins changed the title ~~Optimize document detach operation via Cluster Server~~ Optimize document detachment in Cluster Server Nov 8, 2024

hackerwins merged commit 38526ff into main Nov 8, 2024
5 checks passed

hackerwins deleted the deactivate-optimize branch November 8, 2024 10:06

coderabbitai bot mentioned this pull request Nov 13, 2024

Add migration script to detach documents from deactivated clients #1062

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize document detachment in Cluster Server #1055

Optimize document detachment in Cluster Server #1055

raararaara commented Oct 31, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 31, 2024 •

edited

Loading

Review failed

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

codecov bot commented Oct 31, 2024 •

edited

Loading

github-actions bot left a comment •

edited

Loading

coderabbitai bot left a comment

hackerwins left a comment

Optimize document detachment in Cluster Server #1055

Optimize document detachment in Cluster Server #1055

Conversation

raararaara commented Oct 31, 2024 • edited by coderabbitai bot Loading

What this PR does / why we need it?

Any background context you want to provide?

Summary by CodeRabbit

Release Notes

coderabbitai bot commented Oct 31, 2024 • edited Loading

Review failed

Walkthrough

Changes

Possibly related issues

Possibly related PRs

Suggested labels

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

codecov bot commented Oct 31, 2024 • edited Loading

Codecov Report

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Go Benchmark

coderabbitai bot left a comment

Choose a reason for hiding this comment

hackerwins left a comment

Choose a reason for hiding this comment

raararaara commented Oct 31, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 31, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

codecov bot commented Oct 31, 2024 •

edited

Loading

github-actions bot left a comment •

edited

Loading