Clear presence when housekeeping process #936

raararaara · 2024-07-22T02:23:45Z

What this PR does / why we need it?

This PR addresses the absence of presence clearing logic in document detach during client deactivation for Housekeeping compared to document detach by the SDK. By implementing server-side presence clearance, this PR ensures that presence is correctly handled during the housekeeping process.

Any background context you want to provide?

The changes include:

Performing document detach with client deactivation and updating the SyncedSeq solely through the database -> making an API call similar to the SDK's document detach
This change allows presence to be cleared through client.detach() during deactivation and detach processes, simplifying the process and omitting the need for updating SyncedSeq separately.
In case of deactivation failure, some documents may not detach completely, but another deactivation attempt is possible.
Consideration for clustering and sharding per document
Taking into account document structures sharded across clusters by keys, the update allows one server to act as a client while multiple servers can perform detach operations.

What are the relevant tickets?

Fixes #602

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

New Features
- Introduced methods for simulating document attachment and client activation, enhancing server-side operations.
- Added functionality for converting internal documents to public documents, improving document management.
- Enhanced client connection capabilities, allowing for improved document and client interaction.
- Implemented configuration options for dynamic gateway service referencing in Kubernetes.
- Added command-line flags for backend gateway address configuration, increasing server flexibility.
Bug Fixes
- Improved error handling for detach requests to ensure system integrity in case of failures.
- Enhanced logic for client deactivation, ensuring the most up-to-date client state is utilized.
Chores
- Streamlined methods for client deactivation, simplifying document status management during operations.
- Expanded configuration options in the Helm chart for better service adaptability.
- Added new constants and configuration fields to improve server setup and management.

coderabbitai · 2024-07-22T02:23:52Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This update enhances error handling and control flow during client detachment and deactivation processes, ensuring system consistency and integrity. Key enhancements include new methods for simulating document attachment and activation, improved backend interactions during client deactivation, and effective presence management. The housekeeping process has been refined to clean up presence more efficiently, addressing potential growth in the presence snapshot.

Changes

File	Change Summary
client/client.go	Added `PretendAttach` and `PretendActivate` methods for simulating attachment and activation; improved error handling in the `Detach` method.
pkg/document/document.go	Introduced `ToDocument` function for converting `InternalDocument` to `Document`; added `setInternalDoc` for better internal document management.
pkg/document/internal_document.go	Added `SyncCheckpoint` and `ToDocument` methods to enhance synchronization and conversion functionalities.
server/clients/clients.go	Modified `Deactivate` function to utilize `*backend.Backend`, ensuring proper client info retrieval and document update logic.
server/rpc/yorkie_server.go	Updated `DeactivateClient` to use `s.backend` instead of `s.backend.DB`, improving backend interactions.
server/server.go	Altered `DeactivateClient` and `RegisterHousekeepingTasks` methods to integrate `rpcAddr`, enhancing client management and housekeeping task registration.
build/charts/yorkie-cluster/templates/yorkie/deployment.yaml	Added `GATEWAY_HOST` environment variable to Kubernetes deployment for flexible service discovery.
build/charts/yorkie-cluster/values.yaml	Introduced a new `gateway` configuration option to enhance service routing capabilities.

Assessment against linked issues

Objective	Addressed	Explanation
Handle Presence Cleanup in Housekeeping Process (#602)	✅
Ensure presence is cleared during document detachment	✅	Presence is now correctly managed during detachment processes.

🐇 In warren deep, where shadows play,
Changes weave a bright array.
With presence clear, and logic sound,
The rabbit hops where joy is found!
🥕✨

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.

github-actions

Go Benchmark

Benchmark suite	Current: `1ea7a8d`	Previous: `4ae640a`	Ratio
`BenchmarkDocument/constructor_test`	`1530` ns/op 1337 B/op 24 allocs/op	`1549` ns/op 1337 B/op 24 allocs/op	`0.99`
`BenchmarkDocument/constructor_test - ns/op`	`1530` ns/op	`1549` ns/op	`0.99`
`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`	`980.1` ns/op 1305 B/op 22 allocs/op	`969.3` ns/op 1305 B/op 22 allocs/op	`1.01`
`BenchmarkDocument/status_test - ns/op`	`980.1` ns/op	`969.3` ns/op	`1.01`
`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`	`7791` ns/op 7273 B/op 132 allocs/op	`7797` ns/op 7273 B/op 132 allocs/op	`1.00`
`BenchmarkDocument/equals_test - ns/op`	`7791` ns/op	`7797` ns/op	`1.00`
`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`	`17122` ns/op 12139 B/op 262 allocs/op	`20586` ns/op 12138 B/op 262 allocs/op	`0.83`
`BenchmarkDocument/nested_update_test - ns/op`	`17122` ns/op	`20586` ns/op	`0.83`
`BenchmarkDocument/nested_update_test - B/op`	`12139` B/op	`12138` B/op	`1.00`
`BenchmarkDocument/nested_update_test - allocs/op`	`262` allocs/op	`262` allocs/op	`1`
`BenchmarkDocument/delete_test`	`22974` ns/op 15364 B/op 341 allocs/op	`23144` ns/op 15363 B/op 341 allocs/op	`0.99`
`BenchmarkDocument/delete_test - ns/op`	`22974` ns/op	`23144` ns/op	`0.99`
`BenchmarkDocument/delete_test - B/op`	`15364` B/op	`15363` B/op	`1.00`
`BenchmarkDocument/delete_test - allocs/op`	`341` allocs/op	`341` allocs/op	`1`
`BenchmarkDocument/object_test`	`8829` ns/op 6817 B/op 120 allocs/op	`8717` ns/op 6817 B/op 120 allocs/op	`1.01`
`BenchmarkDocument/object_test - ns/op`	`8829` ns/op	`8717` ns/op	`1.01`
`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`	`30795` ns/op 11947 B/op 276 allocs/op	`29360` ns/op 11947 B/op 276 allocs/op	`1.05`
`BenchmarkDocument/array_test - ns/op`	`30795` ns/op	`29360` ns/op	`1.05`
`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`	`33399` ns/op 14715 B/op 469 allocs/op	`30837` ns/op 14715 B/op 469 allocs/op	`1.08`
`BenchmarkDocument/text_test - ns/op`	`33399` ns/op	`30837` ns/op	`1.08`
`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`	`29456` ns/op 18420 B/op 484 allocs/op	`29504` ns/op 18423 B/op 484 allocs/op	`1.00`
`BenchmarkDocument/text_composition_test - ns/op`	`29456` ns/op	`29504` ns/op	`1.00`
`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`	`82871` ns/op 38476 B/op 1148 allocs/op	`82340` ns/op 38477 B/op 1148 allocs/op	`1.01`
`BenchmarkDocument/rich_text_test - ns/op`	`82871` ns/op	`82340` ns/op	`1.01`
`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`	`17760` ns/op 10722 B/op 244 allocs/op	`17783` ns/op 10722 B/op 244 allocs/op	`1.00`
`BenchmarkDocument/counter_test - ns/op`	`17760` ns/op	`17783` ns/op	`1.00`
`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`	`1298516` ns/op 870986 B/op 16752 allocs/op	`1297144` ns/op 870921 B/op 16753 allocs/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - ns/op`	`1298516` ns/op	`1297144` ns/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - B/op`	`870986` B/op	`870921` B/op	`1.00`
`BenchmarkDocument/text_edit_gc_100 - allocs/op`	`16752` allocs/op	`16753` allocs/op	`1.00`
`BenchmarkDocument/text_edit_gc_1000`	`50459065` ns/op 50535401 B/op 181713 allocs/op	`50024644` ns/op 50535262 B/op 181716 allocs/op	`1.01`
`BenchmarkDocument/text_edit_gc_1000 - ns/op`	`50459065` ns/op	`50024644` ns/op	`1.01`
`BenchmarkDocument/text_edit_gc_1000 - B/op`	`50535401` B/op	`50535262` B/op	`1.00`
`BenchmarkDocument/text_edit_gc_1000 - allocs/op`	`181713` allocs/op	`181716` allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_100`	`1919207` ns/op 1528773 B/op 15604 allocs/op	`1901148` ns/op 1528771 B/op 15604 allocs/op	`1.01`
`BenchmarkDocument/text_split_gc_100 - ns/op`	`1919207` ns/op	`1901148` ns/op	`1.01`
`BenchmarkDocument/text_split_gc_100 - B/op`	`1528773` B/op	`1528771` B/op	`1.00`
`BenchmarkDocument/text_split_gc_100 - allocs/op`	`15604` allocs/op	`15604` allocs/op	`1`
`BenchmarkDocument/text_split_gc_1000`	`113894841` ns/op 135078233 B/op 182200 allocs/op	`113539833` ns/op 135077032 B/op 182196 allocs/op	`1.00`
`BenchmarkDocument/text_split_gc_1000 - ns/op`	`113894841` ns/op	`113539833` ns/op	`1.00`
`BenchmarkDocument/text_split_gc_1000 - B/op`	`135078233` B/op	`135077032` B/op	`1.00`
`BenchmarkDocument/text_split_gc_1000 - allocs/op`	`182200` allocs/op	`182196` allocs/op	`1.00`
`BenchmarkDocument/text_delete_all_10000`	`17815601` ns/op 10181918 B/op 40671 allocs/op	`16926453` ns/op 10183237 B/op 40674 allocs/op	`1.05`
`BenchmarkDocument/text_delete_all_10000 - ns/op`	`17815601` ns/op	`16926453` ns/op	`1.05`
`BenchmarkDocument/text_delete_all_10000 - B/op`	`10181918` B/op	`10183237` B/op	`1.00`
`BenchmarkDocument/text_delete_all_10000 - allocs/op`	`40671` allocs/op	`40674` allocs/op	`1.00`
`BenchmarkDocument/text_delete_all_100000`	`306971632` ns/op 142687752 B/op 411749 allocs/op	`308578662` ns/op 142678592 B/op 411701 allocs/op	`0.99`
`BenchmarkDocument/text_delete_all_100000 - ns/op`	`306971632` ns/op	`308578662` ns/op	`0.99`
`BenchmarkDocument/text_delete_all_100000 - B/op`	`142687752` B/op	`142678592` B/op	`1.00`
`BenchmarkDocument/text_delete_all_100000 - allocs/op`	`411749` allocs/op	`411701` allocs/op	`1.00`
`BenchmarkDocument/text_100`	`216843` ns/op 120037 B/op 5081 allocs/op	`216580` ns/op 120037 B/op 5081 allocs/op	`1.00`
`BenchmarkDocument/text_100 - ns/op`	`216843` ns/op	`216580` ns/op	`1.00`
`BenchmarkDocument/text_100 - B/op`	`120037` B/op	`120037` B/op	`1`
`BenchmarkDocument/text_100 - allocs/op`	`5081` allocs/op	`5081` allocs/op	`1`
`BenchmarkDocument/text_1000`	`2368709` ns/op 1169024 B/op 50085 allocs/op	`2365268` ns/op 1169023 B/op 50085 allocs/op	`1.00`
`BenchmarkDocument/text_1000 - ns/op`	`2368709` ns/op	`2365268` ns/op	`1.00`
`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`	`1237998` ns/op 1091446 B/op 11832 allocs/op	`1233259` ns/op 1091316 B/op 11831 allocs/op	`1.00`
`BenchmarkDocument/array_1000 - ns/op`	`1237998` ns/op	`1233259` ns/op	`1.00`
`BenchmarkDocument/array_1000 - B/op`	`1091446` B/op	`1091316` B/op	`1.00`
`BenchmarkDocument/array_1000 - allocs/op`	`11832` allocs/op	`11831` allocs/op	`1.00`
`BenchmarkDocument/array_10000`	`13612403` ns/op 9798905 B/op 120292 allocs/op	`13201619` ns/op 9799969 B/op 120296 allocs/op	`1.03`
`BenchmarkDocument/array_10000 - ns/op`	`13612403` ns/op	`13201619` ns/op	`1.03`
`BenchmarkDocument/array_10000 - B/op`	`9798905` B/op	`9799969` B/op	`1.00`
`BenchmarkDocument/array_10000 - allocs/op`	`120292` allocs/op	`120296` allocs/op	`1.00`
`BenchmarkDocument/array_gc_100`	`148323` ns/op 132717 B/op 1260 allocs/op	`148310` ns/op 132724 B/op 1261 allocs/op	`1.00`
`BenchmarkDocument/array_gc_100 - ns/op`	`148323` ns/op	`148310` ns/op	`1.00`
`BenchmarkDocument/array_gc_100 - B/op`	`132717` B/op	`132724` B/op	`1.00`
`BenchmarkDocument/array_gc_100 - allocs/op`	`1260` allocs/op	`1261` allocs/op	`1.00`
`BenchmarkDocument/array_gc_1000`	`1423740` ns/op 1159168 B/op 12877 allocs/op	`1396440` ns/op 1159102 B/op 12876 allocs/op	`1.02`
`BenchmarkDocument/array_gc_1000 - ns/op`	`1423740` ns/op	`1396440` ns/op	`1.02`
`BenchmarkDocument/array_gc_1000 - B/op`	`1159168` B/op	`1159102` B/op	`1.00`
`BenchmarkDocument/array_gc_1000 - allocs/op`	`12877` allocs/op	`12876` allocs/op	`1.00`
`BenchmarkDocument/counter_1000`	`200357` ns/op 193080 B/op 5771 allocs/op	`202334` ns/op 193080 B/op 5771 allocs/op	`0.99`
`BenchmarkDocument/counter_1000 - ns/op`	`200357` ns/op	`202334` ns/op	`0.99`
`BenchmarkDocument/counter_1000 - B/op`	`193080` B/op	`193080` B/op	`1`
`BenchmarkDocument/counter_1000 - allocs/op`	`5771` allocs/op	`5771` allocs/op	`1`
`BenchmarkDocument/counter_10000`	`2175683` ns/op 2088011 B/op 59778 allocs/op	`2199700` ns/op 2088013 B/op 59778 allocs/op	`0.99`
`BenchmarkDocument/counter_10000 - ns/op`	`2175683` ns/op	`2199700` ns/op	`0.99`
`BenchmarkDocument/counter_10000 - B/op`	`2088011` B/op	`2088013` B/op	`1.00`
`BenchmarkDocument/counter_10000 - allocs/op`	`59778` allocs/op	`59778` allocs/op	`1`
`BenchmarkDocument/object_1000`	`1420011` ns/op 1428058 B/op 9849 allocs/op	`1413682` ns/op 1428153 B/op 9849 allocs/op	`1.00`
`BenchmarkDocument/object_1000 - ns/op`	`1420011` ns/op	`1413682` ns/op	`1.00`
`BenchmarkDocument/object_1000 - B/op`	`1428058` B/op	`1428153` B/op	`1.00`
`BenchmarkDocument/object_1000 - allocs/op`	`9849` allocs/op	`9849` allocs/op	`1`
`BenchmarkDocument/object_10000`	`15669855` ns/op 12166326 B/op 100563 allocs/op	`15783586` ns/op 12166732 B/op 100564 allocs/op	`0.99`
`BenchmarkDocument/object_10000 - ns/op`	`15669855` ns/op	`15783586` ns/op	`0.99`
`BenchmarkDocument/object_10000 - B/op`	`12166326` B/op	`12166732` B/op	`1.00`
`BenchmarkDocument/object_10000 - allocs/op`	`100563` allocs/op	`100564` allocs/op	`1.00`
`BenchmarkDocument/tree_100`	`1031818` ns/op 943700 B/op 6101 allocs/op	`1047638` ns/op 943702 B/op 6101 allocs/op	`0.98`
`BenchmarkDocument/tree_100 - ns/op`	`1031818` ns/op	`1047638` ns/op	`0.98`
`BenchmarkDocument/tree_100 - B/op`	`943700` B/op	`943702` B/op	`1.00`
`BenchmarkDocument/tree_100 - allocs/op`	`6101` allocs/op	`6101` allocs/op	`1`
`BenchmarkDocument/tree_1000`	`75736274` ns/op 86460373 B/op 60115 allocs/op	`77148387` ns/op 86460333 B/op 60115 allocs/op	`0.98`
`BenchmarkDocument/tree_1000 - ns/op`	`75736274` ns/op	`77148387` ns/op	`0.98`
`BenchmarkDocument/tree_1000 - B/op`	`86460373` B/op	`86460333` B/op	`1.00`
`BenchmarkDocument/tree_1000 - allocs/op`	`60115` allocs/op	`60115` allocs/op	`1`
`BenchmarkDocument/tree_10000`	`9768532453` ns/op 8580666432 B/op 600201 allocs/op	`9715671041` ns/op 8580668976 B/op 600226 allocs/op	`1.01`
`BenchmarkDocument/tree_10000 - ns/op`	`9768532453` ns/op	`9715671041` ns/op	`1.01`
`BenchmarkDocument/tree_10000 - B/op`	`8580666432` B/op	`8580668976` B/op	`1.00`
`BenchmarkDocument/tree_10000 - allocs/op`	`600201` allocs/op	`600226` allocs/op	`1.00`
`BenchmarkDocument/tree_delete_all_1000`	`76590265` ns/op 87509176 B/op 75264 allocs/op	`80678138` ns/op 87509409 B/op 75264 allocs/op	`0.95`
`BenchmarkDocument/tree_delete_all_1000 - ns/op`	`76590265` ns/op	`80678138` ns/op	`0.95`
`BenchmarkDocument/tree_delete_all_1000 - B/op`	`87509176` B/op	`87509409` B/op	`1.00`
`BenchmarkDocument/tree_delete_all_1000 - allocs/op`	`75264` allocs/op	`75264` allocs/op	`1`
`BenchmarkDocument/tree_edit_gc_100`	`3820383` ns/op 4146633 B/op 15140 allocs/op	`3960878` ns/op 4146779 B/op 15141 allocs/op	`0.96`
`BenchmarkDocument/tree_edit_gc_100 - ns/op`	`3820383` ns/op	`3960878` ns/op	`0.96`
`BenchmarkDocument/tree_edit_gc_100 - B/op`	`4146633` B/op	`4146779` B/op	`1.00`
`BenchmarkDocument/tree_edit_gc_100 - allocs/op`	`15140` allocs/op	`15141` allocs/op	`1.00`
`BenchmarkDocument/tree_edit_gc_1000`	`316301091` ns/op 383826964 B/op 154865 allocs/op	`314875560` ns/op 383824892 B/op 154860 allocs/op	`1.00`
`BenchmarkDocument/tree_edit_gc_1000 - ns/op`	`316301091` ns/op	`314875560` ns/op	`1.00`
`BenchmarkDocument/tree_edit_gc_1000 - B/op`	`383826964` B/op	`383824892` B/op	`1.00`
`BenchmarkDocument/tree_edit_gc_1000 - allocs/op`	`154865` allocs/op	`154860` allocs/op	`1.00`
`BenchmarkDocument/tree_split_gc_100`	`2559288` ns/op 2412582 B/op 11125 allocs/op	`2600369` ns/op 2412515 B/op 11125 allocs/op	`0.98`
`BenchmarkDocument/tree_split_gc_100 - ns/op`	`2559288` ns/op	`2600369` ns/op	`0.98`
`BenchmarkDocument/tree_split_gc_100 - B/op`	`2412582` B/op	`2412515` B/op	`1.00`
`BenchmarkDocument/tree_split_gc_100 - allocs/op`	`11125` allocs/op	`11125` allocs/op	`1`
`BenchmarkDocument/tree_split_gc_1000`	`185198690` ns/op 222251754 B/op 122006 allocs/op	`189117419` ns/op 222249912 B/op 121990 allocs/op	`0.98`
`BenchmarkDocument/tree_split_gc_1000 - ns/op`	`185198690` ns/op	`189117419` ns/op	`0.98`
`BenchmarkDocument/tree_split_gc_1000 - B/op`	`222251754` B/op	`222249912` B/op	`1.00`
`BenchmarkDocument/tree_split_gc_1000 - allocs/op`	`122006` allocs/op	`121990` allocs/op	`1.00`
`BenchmarkRPC/client_to_server`	`354705639` ns/op 17226525 B/op 170848 allocs/op	`357022218` ns/op 17129704 B/op 171446 allocs/op	`0.99`
`BenchmarkRPC/client_to_server - ns/op`	`354705639` ns/op	`357022218` ns/op	`0.99`
`BenchmarkRPC/client_to_server - B/op`	`17226525` B/op	`17129704` B/op	`1.01`
`BenchmarkRPC/client_to_server - allocs/op`	`170848` allocs/op	`171446` allocs/op	`1.00`
`BenchmarkRPC/client_to_client_via_server`	`660356794` ns/op 34636304 B/op 334749 allocs/op	`659779438` ns/op 34755660 B/op 335349 allocs/op	`1.00`
`BenchmarkRPC/client_to_client_via_server - ns/op`	`660356794` ns/op	`659779438` ns/op	`1.00`
`BenchmarkRPC/client_to_client_via_server - B/op`	`34636304` B/op	`34755660` B/op	`1.00`
`BenchmarkRPC/client_to_client_via_server - allocs/op`	`334749` allocs/op	`335349` allocs/op	`1.00`
`BenchmarkRPC/attach_large_document`	`2274158393` ns/op 3613459928 B/op 14320 allocs/op	`2231916118` ns/op 3604715200 B/op 14410 allocs/op	`1.02`
`BenchmarkRPC/attach_large_document - ns/op`	`2274158393` ns/op	`2231916118` ns/op	`1.02`
`BenchmarkRPC/attach_large_document - B/op`	`3613459928` B/op	`3604715200` B/op	`1.00`
`BenchmarkRPC/attach_large_document - allocs/op`	`14320` allocs/op	`14410` allocs/op	`0.99`
`BenchmarkRPC/adminCli_to_server`	`537097757` ns/op 35965788 B/op 289594 allocs/op	`539083220` ns/op 35949996 B/op 289538 allocs/op	`1.00`
`BenchmarkRPC/adminCli_to_server - ns/op`	`537097757` ns/op	`539083220` ns/op	`1.00`
`BenchmarkRPC/adminCli_to_server - B/op`	`35965788` B/op	`35949996` B/op	`1.00`
`BenchmarkRPC/adminCli_to_server - allocs/op`	`289594` allocs/op	`289538` allocs/op	`1.00`
`BenchmarkLocker`	`65.39` ns/op 16 B/op 1 allocs/op	`64.48` ns/op 16 B/op 1 allocs/op	`1.01`
`BenchmarkLocker - ns/op`	`65.39` ns/op	`64.48` 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`	`40.58` ns/op 0 B/op 0 allocs/op	`39.78` ns/op 0 B/op 0 allocs/op	`1.02`
`BenchmarkLockerParallel - ns/op`	`40.58` ns/op	`39.78` ns/op	`1.02`
`BenchmarkLockerParallel - B/op`	`0` B/op	`0` B/op	`1`
`BenchmarkLockerParallel - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkLockerMoreKeys`	`142.7` ns/op 15 B/op 0 allocs/op	`162.3` ns/op 15 B/op 0 allocs/op	`0.88`
`BenchmarkLockerMoreKeys - ns/op`	`142.7` ns/op	`162.3` ns/op	`0.88`
`BenchmarkLockerMoreKeys - B/op`	`15` B/op	`15` B/op	`1`
`BenchmarkLockerMoreKeys - allocs/op`	`0` allocs/op	`0` allocs/op	`1`
`BenchmarkChange/Push_10_Changes`	`3815418` ns/op 116743 B/op 1214 allocs/op	`3875350` ns/op 116948 B/op 1214 allocs/op	`0.98`
`BenchmarkChange/Push_10_Changes - ns/op`	`3815418` ns/op	`3875350` ns/op	`0.98`
`BenchmarkChange/Push_10_Changes - B/op`	`116743` B/op	`116948` B/op	`1.00`
`BenchmarkChange/Push_10_Changes - allocs/op`	`1214` allocs/op	`1214` allocs/op	`1`
`BenchmarkChange/Push_100_Changes`	`15688516` ns/op 569754 B/op 6585 allocs/op	`15938762` ns/op 571609 B/op 6585 allocs/op	`0.98`
`BenchmarkChange/Push_100_Changes - ns/op`	`15688516` ns/op	`15938762` ns/op	`0.98`
`BenchmarkChange/Push_100_Changes - B/op`	`569754` B/op	`571609` B/op	`1.00`
`BenchmarkChange/Push_100_Changes - allocs/op`	`6585` allocs/op	`6585` allocs/op	`1`
`BenchmarkChange/Push_1000_Changes`	`129021587` ns/op 5265827 B/op 63078 allocs/op	`128283783` ns/op 5390909 B/op 63081 allocs/op	`1.01`
`BenchmarkChange/Push_1000_Changes - ns/op`	`129021587` ns/op	`128283783` ns/op	`1.01`
`BenchmarkChange/Push_1000_Changes - B/op`	`5265827` B/op	`5390909` B/op	`0.98`
`BenchmarkChange/Push_1000_Changes - allocs/op`	`63078` allocs/op	`63081` allocs/op	`1.00`
`BenchmarkChange/Pull_10_Changes`	`3112768` ns/op 102482 B/op 1018 allocs/op	`3085917` ns/op 102318 B/op 1019 allocs/op	`1.01`
`BenchmarkChange/Pull_10_Changes - ns/op`	`3112768` ns/op	`3085917` ns/op	`1.01`
`BenchmarkChange/Pull_10_Changes - B/op`	`102482` B/op	`102318` B/op	`1.00`
`BenchmarkChange/Pull_10_Changes - allocs/op`	`1018` allocs/op	`1019` allocs/op	`1.00`
`BenchmarkChange/Pull_100_Changes`	`4691944` ns/op 266255 B/op 3488 allocs/op	`4653244` ns/op 266393 B/op 3489 allocs/op	`1.01`
`BenchmarkChange/Pull_100_Changes - ns/op`	`4691944` ns/op	`4653244` ns/op	`1.01`
`BenchmarkChange/Pull_100_Changes - B/op`	`266255` B/op	`266393` B/op	`1.00`
`BenchmarkChange/Pull_100_Changes - allocs/op`	`3488` allocs/op	`3489` allocs/op	`1.00`
`BenchmarkChange/Pull_1000_Changes`	`9182040` ns/op 1489154 B/op 29858 allocs/op	`8734569` ns/op 1489803 B/op 29867 allocs/op	`1.05`
`BenchmarkChange/Pull_1000_Changes - ns/op`	`9182040` ns/op	`8734569` ns/op	`1.05`
`BenchmarkChange/Pull_1000_Changes - B/op`	`1489154` B/op	`1489803` B/op	`1.00`
`BenchmarkChange/Pull_1000_Changes - allocs/op`	`29858` allocs/op	`29867` allocs/op	`1.00`
`BenchmarkSnapshot/Push_3KB_snapshot`	`18415769` ns/op 698976 B/op 6586 allocs/op	`18691314` ns/op 710356 B/op 6587 allocs/op	`0.99`
`BenchmarkSnapshot/Push_3KB_snapshot - ns/op`	`18415769` ns/op	`18691314` ns/op	`0.99`
`BenchmarkSnapshot/Push_3KB_snapshot - B/op`	`698976` B/op	`710356` B/op	`0.98`
`BenchmarkSnapshot/Push_3KB_snapshot - allocs/op`	`6586` allocs/op	`6587` allocs/op	`1.00`
`BenchmarkSnapshot/Push_30KB_snapshot`	`133594125` ns/op 5560172 B/op 63078 allocs/op	`129614800` ns/op 5588012 B/op 63080 allocs/op	`1.03`
`BenchmarkSnapshot/Push_30KB_snapshot - ns/op`	`133594125` ns/op	`129614800` ns/op	`1.03`
`BenchmarkSnapshot/Push_30KB_snapshot - B/op`	`5560172` B/op	`5588012` B/op	`1.00`
`BenchmarkSnapshot/Push_30KB_snapshot - allocs/op`	`63078` allocs/op	`63080` allocs/op	`1.00`
`BenchmarkSnapshot/Pull_3KB_snapshot`	`6923173` ns/op 919572 B/op 15519 allocs/op	`6800608` ns/op 921145 B/op 15522 allocs/op	`1.02`
`BenchmarkSnapshot/Pull_3KB_snapshot - ns/op`	`6923173` ns/op	`6800608` ns/op	`1.02`
`BenchmarkSnapshot/Pull_3KB_snapshot - B/op`	`919572` B/op	`921145` B/op	`1.00`
`BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op`	`15519` allocs/op	`15522` allocs/op	`1.00`
`BenchmarkSnapshot/Pull_30KB_snapshot`	`15437373` ns/op 7153223 B/op 150123 allocs/op	`15443953` ns/op 7150322 B/op 150120 allocs/op	`1.00`
`BenchmarkSnapshot/Pull_30KB_snapshot - ns/op`	`15437373` ns/op	`15443953` ns/op	`1.00`
`BenchmarkSnapshot/Pull_30KB_snapshot - B/op`	`7153223` B/op	`7150322` B/op	`1.00`
`BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op`	`150123` allocs/op	`150120` allocs/op	`1.00`
`BenchmarkSync/memory_sync_10_test`	`7229` ns/op 1286 B/op 38 allocs/op	`7266` ns/op 1286 B/op 38 allocs/op	`0.99`
`BenchmarkSync/memory_sync_10_test - ns/op`	`7229` ns/op	`7266` ns/op	`0.99`
`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`	`52078` ns/op 8639 B/op 273 allocs/op	`51934` ns/op 8632 B/op 272 allocs/op	`1.00`
`BenchmarkSync/memory_sync_100_test - ns/op`	`52078` ns/op	`51934` ns/op	`1.00`
`BenchmarkSync/memory_sync_100_test - B/op`	`8639` B/op	`8632` B/op	`1.00`
`BenchmarkSync/memory_sync_100_test - allocs/op`	`273` allocs/op	`272` allocs/op	`1.00`
`BenchmarkSync/memory_sync_1000_test`	`596382` ns/op 74220 B/op 2113 allocs/op	`587913` ns/op 74229 B/op 2113 allocs/op	`1.01`
`BenchmarkSync/memory_sync_1000_test - ns/op`	`596382` ns/op	`587913` ns/op	`1.01`
`BenchmarkSync/memory_sync_1000_test - B/op`	`74220` B/op	`74229` B/op	`1.00`
`BenchmarkSync/memory_sync_1000_test - allocs/op`	`2113` allocs/op	`2113` allocs/op	`1`
`BenchmarkSync/memory_sync_10000_test`	`7847813` ns/op 734726 B/op 20226 allocs/op	`7400660` ns/op 735273 B/op 20214 allocs/op	`1.06`
`BenchmarkSync/memory_sync_10000_test - ns/op`	`7847813` ns/op	`7400660` ns/op	`1.06`
`BenchmarkSync/memory_sync_10000_test - B/op`	`734726` B/op	`735273` B/op	`1.00`
`BenchmarkSync/memory_sync_10000_test - allocs/op`	`20226` allocs/op	`20214` allocs/op	`1.00`
`BenchmarkTextEditing`	`5568360049` ns/op 3901890352 B/op 18743132 allocs/op	`4889188948` ns/op 3901924192 B/op 18743195 allocs/op	`1.14`
`BenchmarkTextEditing - ns/op`	`5568360049` ns/op	`4889188948` ns/op	`1.14`
`BenchmarkTextEditing - B/op`	`3901890352` B/op	`3901924192` B/op	`1.00`
`BenchmarkTextEditing - allocs/op`	`18743132` allocs/op	`18743195` allocs/op	`1.00`
`BenchmarkTree/10000_vertices_to_protobuf`	`3570428` ns/op 6262996 B/op 70025 allocs/op	`3446065` ns/op 6263016 B/op 70025 allocs/op	`1.04`
`BenchmarkTree/10000_vertices_to_protobuf - ns/op`	`3570428` ns/op	`3446065` ns/op	`1.04`
`BenchmarkTree/10000_vertices_to_protobuf - B/op`	`6262996` B/op	`6263016` B/op	`1.00`
`BenchmarkTree/10000_vertices_to_protobuf - allocs/op`	`70025` allocs/op	`70025` allocs/op	`1`
`BenchmarkTree/10000_vertices_from_protobuf`	`161922777` ns/op 442171475 B/op 290039 allocs/op	`149737156` ns/op 442171406 B/op 290038 allocs/op	`1.08`
`BenchmarkTree/10000_vertices_from_protobuf - ns/op`	`161922777` ns/op	`149737156` ns/op	`1.08`
`BenchmarkTree/10000_vertices_from_protobuf - B/op`	`442171475` B/op	`442171406` B/op	`1.00`
`BenchmarkTree/10000_vertices_from_protobuf - allocs/op`	`290039` allocs/op	`290038` allocs/op	`1.00`
`BenchmarkTree/20000_vertices_to_protobuf`	`7855497` ns/op 12716982 B/op 140028 allocs/op	`7485911` ns/op 12716915 B/op 140028 allocs/op	`1.05`
`BenchmarkTree/20000_vertices_to_protobuf - ns/op`	`7855497` ns/op	`7485911` ns/op	`1.05`
`BenchmarkTree/20000_vertices_to_protobuf - B/op`	`12716982` B/op	`12716915` B/op	`1.00`
`BenchmarkTree/20000_vertices_to_protobuf - allocs/op`	`140028` allocs/op	`140028` allocs/op	`1`
`BenchmarkTree/20000_vertices_from_protobuf`	`716015482` ns/op 1697272676 B/op 580049 allocs/op	`677321812` ns/op 1697267944 B/op 580043 allocs/op	`1.06`
`BenchmarkTree/20000_vertices_from_protobuf - ns/op`	`716015482` ns/op	`677321812` ns/op	`1.06`
`BenchmarkTree/20000_vertices_from_protobuf - B/op`	`1697272676` B/op	`1697267944` B/op	`1.00`
`BenchmarkTree/20000_vertices_from_protobuf - allocs/op`	`580049` allocs/op	`580043` allocs/op	`1.00`
`BenchmarkTree/30000_vertices_to_protobuf`	`13042787` ns/op 19318246 B/op 210030 allocs/op	`12087022` ns/op 19318328 B/op 210030 allocs/op	`1.08`
`BenchmarkTree/30000_vertices_to_protobuf - ns/op`	`13042787` ns/op	`12087022` ns/op	`1.08`
`BenchmarkTree/30000_vertices_to_protobuf - B/op`	`19318246` B/op	`19318328` B/op	`1.00`
`BenchmarkTree/30000_vertices_to_protobuf - allocs/op`	`210030` allocs/op	`210030` allocs/op	`1`
`BenchmarkTree/30000_vertices_from_protobuf`	`1698875284` ns/op 3752062456 B/op 870147 allocs/op	`1624417393` ns/op 3752053464 B/op 870140 allocs/op	`1.05`
`BenchmarkTree/30000_vertices_from_protobuf - ns/op`	`1698875284` ns/op	`1624417393` ns/op	`1.05`
`BenchmarkTree/30000_vertices_from_protobuf - B/op`	`3752062456` B/op	`3752053464` B/op	`1.00`
`BenchmarkTree/30000_vertices_from_protobuf - allocs/op`	`870147` allocs/op	`870140` allocs/op	`1.00`

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

codecov · 2024-07-22T02:37:56Z

Codecov Report

Attention: Patch coverage is 15.55556% with 114 lines in your changes missing coverage. Please review.

Project coverage is 48.19%. Comparing base (a4ce314) to head (1ea7a8d).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
server/rpc/system_server.go	6.09%	77 Missing ⚠️
pkg/document/internal_document.go	0.00%	12 Missing ⚠️
server/packs/packs.go	0.00%	8 Missing ⚠️
server/backend/config.go	0.00%	5 Missing ⚠️
server/rpc/yorkie_server.go	57.14%	3 Missing ⚠️
server/server.go	0.00%	3 Missing ⚠️
client/client.go	0.00%	2 Missing ⚠️
pkg/document/document.go	0.00%	2 Missing ⚠️
server/backend/database/mongo/client.go	80.00%	0 Missing and 1 partial ⚠️
server/packs/pushpull.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #936      +/-   ##
==========================================
- Coverage   51.47%   48.19%   -3.29%     
==========================================
  Files          71       80       +9     
  Lines       10575    11442     +867     
==========================================
+ Hits         5444     5514      +70     
- Misses       4585     5380     +795     
- Partials      546      548       +2

Flag	Coverage Δ
	`48.19% <15.55%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

coderabbitai

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 a4ce314 and 74bca05.

Files selected for processing (7)

client/client.go (1 hunks)
pkg/document/document.go (2 hunks)
server/clients/clients.go (3 hunks)
server/clients/housekeeping.go (1 hunks)
server/rpc/yorkie_server.go (1 hunks)
server/server.go (1 hunks)
test/integration/presence_test.go (1 hunks)

Additional comments not posted (10)

server/clients/housekeeping.go (1)
72-72: Verify the correctness of passing be instead of be.DB.

Ensure that the Deactivate function is updated to handle the be object correctly and that all necessary properties and methods are accessible.

Verification successful

Verified the correctness of passing be instead of be.DB.

The Deactivate function in server/clients/clients.go is updated to accept the be object, confirming that the change in DeactivateInactives is appropriate.

server/clients/clients.go: func Deactivate(ctx context.Context, be *backend.Backend, refKey types.ClientRefKey) (*database.ClientInfo, error)
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that the `Deactivate` function is updated to handle the `be` object correctly.

# Test: Search for the `Deactivate` function definition. Expect: The function should accept `be` instead of `be.DB`.
rg --type go -A 5 'func Deactivate(ctx context.Context, be *backend.Backend' 
Length of output: 201

Script:
#!/bin/bash
# Description: Verify that the `Deactivate` function is updated to handle the `be` object correctly.

# Test: Search for the `Deactivate` function definition. Expect: The function should accept `be` instead of `be.DB`.
rg --type go -A 5 'func Deactivate(ctx context.Context, be \*backend.Backend)' 
Length of output: 80

Script:
#!/bin/bash
# Description: Locate the `Deactivate` function and inspect its definition.

# Test: Search for the `Deactivate` function definition in Go files.
rg --type go 'func Deactivate' -A 10
Length of output: 1645
server/clients/clients.go (3)
64-67: Handle potential errors when fetching client information.

Ensure that the error handling logic is robust when fetching client information using FindClientInfoByRefKey.

Verification successful

Handle potential errors when fetching client information.

The error handling logic for FindClientInfoByRefKey is robust, as it consistently returns nil and the error when an error is encountered.

server/clients/clients.go, lines 64-67
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that the error handling logic is robust when fetching client information.

# Test: Search for the `FindClientInfoByRefKey` function calls. Expect: Proper error handling should be in place.
rg --type go -A 5 'FindClientInfoByRefKey' 
Length of output: 9360
101-132: Verify document update and presence clearing logic.

Ensure that the document update and presence clearing logic is correctly implemented and that the PushPull operation is successful.

Line range hint 55-132:
Verify the correctness of passing be instead of db.

Ensure that the Deactivate function handles the be object correctly and that all necessary properties and methods are accessible.
server/server.go (1)
153-153: Verify the correctness of passing r.backend instead of r.backend.DB.

Ensure that the Deactivate function is updated to handle the r.backend object correctly and that all necessary properties and methods are accessible.
Verification successful

Verified the correctness of passing r.backend instead of r.backend.DB.

The Deactivate function in server/clients/clients.go correctly accepts r.backend (of type *backend.Backend), ensuring that all necessary properties and methods are accessible.
server/clients/clients.go:
func Deactivate(
	ctx context.Context,
	be *backend.Backend,
	refKey types.ClientRefKey,
) (*database.ClientInfo, error) {
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify that the `Deactivate` function is updated to handle the `r.backend` object correctly.

# Test: Search for the `Deactivate` function definition. Expect: The function should accept `r.backend` instead of `r.backend.DB`.
rg --type go -A 5 'func Deactivate(ctx context.Context, be *backend.Backend' 
Length of output: 201

Script:
#!/bin/bash
# Description: Verify that the `Deactivate` function is updated to handle the `r.backend` object correctly.

# Test: Search for the `Deactivate` function definition. Expect: The function should accept `r.backend` instead of `r.backend.DB`.
rg --type go -A 5 'func Deactivate(ctx context.Context, be $_'
Length of output: 173

Script:
#!/bin/bash
# Description: Verify that the `Deactivate` function is updated to handle the `r.backend` object correctly.

# Test: Search for the `Deactivate` function definition. Expect: The function should accept `r.backend` instead of `r.backend.DB`.
rg --type go -A 5 'func Deactivate(ctx context.Context, be .*'
Length of output: 173

Script:
#!/bin/bash
# Description: Verify that the `Deactivate` function is updated to handle the `r.backend` object correctly.

# Test: Search for the `Deactivate` function definition. Expect: The function should accept `r.backend` instead of `r.backend.DB`.
rg --type go -A 5 'func Deactivate$ctx context.Context, be .*$'
Length of output: 67

Script:
#!/bin/bash
# Description: Verify that the `Deactivate` function is updated to handle the `r.backend` object correctly.

# Test: Search for the `Deactivate` function definition. Expect: The function should accept `r.backend` instead of `r.backend.DB`.
rg --type go -A 5 'func Deactivate'
Length of output: 755
pkg/document/document.go (2)
140-147: LGTM! Consider adding options parameter.

The ToDocument function correctly initializes the Document struct. To make the function more versatile, consider adding an options parameter.
func ToDocument(internalDocument *InternalDocument) *Document {
	return &Document{
		doc: internalDocument,
+		options: Options{},
	}
}
181-182: Implement logic to revert change if detach request fails.

The comment correctly indicates the need to revert the change if the detach request fails. Consider implementing this logic.
// If the detach request fails, you must revert the
// change before creating the `presenceClear` change.
if err := c.Execute(d.doc.root, d.doc.presences); err != nil {
	// Revert the change here
	return err
}
test/integration/presence_test.go (1)
470-491: LGTM! Consider adding assertions for d1.

The test case correctly validates the presence management logic. To make it more comprehensive, consider adding assertions to verify the presence count of d1 after deactivation.
assert.NoError(t, defaultServer.DeactivateClient(ctx, c1))
assert.NoError(t, c2.Sync(ctx))

+ assert.Equal(t, 1, len(d1.AllPresences()))
assert.Equal(t, 1, len(d2.AllPresences()))
server/rpc/yorkie_server.go (1)

97-97: LGTM! Ensure compatibility with dependent functionalities.

The change to use s.backend instead of s.backend.DB in the DeactivateClient method looks good. Ensure that all dependent functionalities and integrations are compatible with this new parameter structure.

client/client.go (1)

395-396: Implement a rollback mechanism for failed detach requests.

The TODO comment indicates the need for a rollback mechanism if the detach request fails. This is crucial to maintain consistency and integrity. Consider implementing this mechanism to revert any changes made prior to the detach request.

Ensure that the rollback mechanism is implemented and tested thoroughly.

coderabbitai

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)

server/clients/clients.go (1)

105-110: Track the TODO task for future improvements.

The TODO comment provides context for future improvements. Consider tracking this task in a project management tool.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 74bca05 and 3409bf1.

Files selected for processing (7)

client/client.go (1 hunks)
pkg/document/document.go (1 hunks)
pkg/document/internal_document.go (2 hunks)
server/clients/clients.go (2 hunks)
server/documents/documents.go (4 hunks)
server/packs/packs.go (2 hunks)
server/packs/pushpull.go (1 hunks)

Files skipped from review as they are similar to previous changes (1)

client/client.go

Additional comments not posted (22)

server/clients/clients.go (11)

25-28: LGTM!

The new imports are necessary for the updated logic in the Deactivate function.

Also applies to: 30-30

54-56: LGTM!

The function signature update to use *backend.Backend allows for more intricate interactions with the backend.

57-66: Ensure proper error handling for client information retrieval.

The error handling for retrieving client information is appropriate. Ensure that similar error handling is applied consistently throughout the function.

68-72: Ensure proper error handling for actor ID conversion.

The error handling for converting client ID to actor ID is appropriate. Ensure that similar error handling is applied consistently throughout the function.

74-78: Ensure proper error handling for project information retrieval.

The error handling for retrieving project information is appropriate. Ensure that similar error handling is applied consistently throughout the function.

80-84: Ensure document detachment logic handles all attached documents.

The loop correctly iterates over all documents associated with the client. Ensure that the condition info.Status != database.DocumentAttached accurately identifies documents that need to be detached.

85-92: Ensure proper error handling for document information retrieval.

The error handling for retrieving document information is appropriate. Ensure that similar error handling is applied consistently throughout the function.

93-96: Ensure proper error handling for building document for checkpoint.

The error handling for building the document for the checkpoint is appropriate. Ensure that similar error handling is applied consistently throughout the function.

98-103: Ensure proper error handling for document state update.

The error handling for updating the document state by clearing the client's presence is appropriate. Ensure that similar error handling is applied consistently throughout the function.

105-114: Ensure proper error handling for push-pull operation.

The error handling for the push-pull operation is appropriate. Ensure that similar error handling is applied consistently throughout the function.

119-123: Ensure proper error handling for client deactivation.

The error handling for deactivating the client is appropriate. Ensure that similar error handling is applied consistently throughout the function.

server/packs/pushpull.go (1)

142-142: LGTM! Verify the behavior of the new function.

The change to use BuildInternalDocForServerSeq suggests a shift in the logic or source of the document being constructed. Ensure that the new function behaves as expected.

server/documents/documents.go (4)

74-74: LGTM! Verify the behavior of the new function.

The change to use BuildInternalDocForServerSeq suggests a shift in the logic or source of the document being constructed. Ensure that the new function behaves as expected.

105-105: LGTM! Verify the behavior of the new function.

The change to use BuildInternalDocForServerSeq suggests a shift in the logic or source of the document being constructed. Ensure that the new function behaves as expected.

138-138: LGTM! Verify the behavior of the new function.

The change to use BuildInternalDocForServerSeq suggests a shift in the logic or source of the document being constructed. Ensure that the new function behaves as expected.

182-182: LGTM! Verify the behavior of the new function.

The change to use BuildInternalDocForServerSeq suggests a shift in the logic or source of the document being constructed. Ensure that the new function behaves as expected.
server/packs/packs.go (3)
33-33: Import statement approved.

The import for github.com/yorkie-team/yorkie/pkg/document/time is necessary for handling the actorID parameter in the updated BuildDocForCheckpoint function.

233-233: Function repositioning approved.

The function BuildInternalDocForServerSeq has been moved to follow BuildDocForCheckpoint, suggesting a logical grouping of related functionalities.

214-231: Function changes approved.

The function BuildDocumentForServerSeq has been renamed to BuildDocForCheckpoint with additional parameters clientSeq and actorID. These changes enhance the function's capability to manage document states in relation to checkpoints.

Ensure that all calls to this function match the new signature.

Verification successful

Verification successful.

All instances of the function BuildDocForCheckpoint in the codebase match the new signature.

server/clients/clients.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `BuildDocForCheckpoint` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'BuildDocForCheckpoint'
Length of output: 785

Script:
#!/bin/bash
# Description: Verify all function calls to `BuildDocForCheckpoint` match the new signature.

# Test: Search for the function usage across the entire codebase. Expect: Only occurrences of the new signature.
rg --type go -A 5 'BuildDocForCheckpoint'
Length of output: 785
pkg/document/internal_document.go (2)

139-149: Method changes approved.

The method SyncCheckpoint synchronizes the checkpoint and change ID with the specified server and client sequences. The logic appears correct and aligns with the intended functionality.

388-393: Method changes approved.

The method ToDocument converts the InternalDocument instance into a Document object. The logic appears correct and aligns with the intended functionality.

pkg/document/document.go (1)

457-461: Method changes approved.

The method setInternalDoc allows setting an internal document reference within an instance of Document. The logic appears correct and aligns with the intended functionality.

coderabbitai

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 3409bf1 and e4ded21.

Files selected for processing (3)

server/backend/database/memory/database.go (1 hunks)
server/backend/database/mongo/client.go (1 hunks)
server/backend/database/testcases/testcases.go (1 hunks)

Files skipped from review due to trivial changes (1)

server/backend/database/testcases/testcases.go

Additional comments not posted (2)

server/backend/database/memory/database.go (1)
Line range hint 523-537:
Verify presence clearing logic after detaching documents.

The removal of the loop that detached documents simplifies the control flow. However, ensure that the presence clearing logic is correctly handled elsewhere to avoid potential issues with attached documents.

Verification successful

Verify presence clearing logic after detaching documents.

The presence clearing logic is correctly handled in the codebase. The ApplyChanges method in InternalDocument manages the PresenceChange with a Clear type, ensuring that users are removed from online clients and the 'unwatched' event is triggered. The removal of the loop in the DeactivateClient method does not introduce any issues.

pkg/document/internal_document.go: ApplyChanges method handles PresenceChange with Clear type.

pkg/document/innerpresence/presence.go: Defines PresenceChange and Clear type.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify presence clearing logic after detaching documents.

# Test: Search for presence clearing logic in the codebase. Expect: Appropriate presence clearing logic should be present.
rg --type go -A 5 'PresenceChange{type: .clear.}'
Length of output: 178

Script:
#!/bin/bash
# Description: Verify presence clearing logic after detaching documents.

# Test: Search for presence clearing logic in the codebase. Expect: Appropriate presence clearing logic should be present.
rg --type go -A 5 'PresenceChange{type: "clear"}'
Length of output: 178

Script:
#!/bin/bash
# Description: Verify presence clearing logic after detaching documents.

# Test: Search for presence clearing logic in the codebase. Expect: Appropriate presence clearing logic should be present.
rg --type go -A 5 'PresenceChange' | rg 'clear'
Length of output: 315

Script:
#!/bin/bash
# Description: Extract relevant code to verify presence clearing logic.

# Extract code from pkg/document/internal_document.go
rg --type go -A 10 -B 5 'PresenceChange' pkg/document/internal_document.go

# Extract code from pkg/document/innerpresence/presence.go
rg --type go -A 10 -B 5 'PresenceChange' pkg/document/innerpresence/presence.go
Length of output: 2585
server/backend/database/mongo/client.go (1)

Line range hint 523-537:
Verify document status management after client deactivation.

The removal of the document status update logic simplifies the method. However, ensure that the document status management is correctly handled elsewhere to avoid potential issues with document statuses.

coderabbitai

Actionable comments posted: 3

Outside diff range, codebase verification and nitpick comments (1)

server/backend/database/client_info.go (1)
287-287: Consider adding a function comment.

Adding a comment to describe the purpose and usage of this function would improve code readability and maintainability.
+// ToClient creates a new client connection using the provided RPC address and project key.
+// It sets the client to active status and assigns an actor ID derived from the ClientInfo instance's ID.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e4ded21 and 13211b1.

Files selected for processing (6)

client/client.go (2 hunks)
server/backend/database/client_info.go (2 hunks)
server/clients/clients.go (2 hunks)
server/clients/housekeeping.go (2 hunks)
server/rpc/yorkie_server.go (1 hunks)
server/server.go (3 hunks)

Files skipped from review as they are similar to previous changes (5)

client/client.go
server/clients/clients.go
server/clients/housekeeping.go
server/rpc/yorkie_server.go
server/server.go

Additional comments not posted (1)

server/backend/database/client_info.go (1)
305-305: LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to ToClient are correctly implemented.

Verification successful

The ToClient function is defined in server/backend/database/client_info.go and is used in server/clients/clients.go.

To ensure that the function usage is correctly implemented, we need to inspect the relevant lines in server/clients/clients.go for proper error handling and subsequent logic.

Function usage of ToClient is correctly implemented.

The function ToClient is used in server/clients/clients.go with proper error handling and subsequent logic. The implementation aligns with typical Go error handling practices and ensures that the code proceeds correctly only if no error occurs.

server/clients/clients.go: Lines around the usage of ToClient show proper error handling and logical flow.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify all function calls to `ToClient` are correctly implemented.

# Test: Search for the function usage. Expect: Correct implementation of the new function.
rg --type go -A 5 $'ToClient'
Length of output: 873

Script:
#!/bin/bash
# Description: Inspect the relevant lines in `server/clients/clients.go` to ensure the `ToClient` function is used correctly.

# Display the relevant lines around the usage of `ToClient` in `server/clients/clients.go`
rg --type go -A 20 $'clientInfo.ToClient' server/clients/clients.go
Length of output: 642

server/backend/database/client_info.go

…nt from clientInfo

…vironment

coderabbitai

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)

client/client.go (1)

385-386: TODO: Revert local presence clearing if server detachment fails.

The comment indicates a need to handle the scenario where local presence clearing must be reverted if the server fails to detach the document.

Acknowledge this TODO item for future development to improve error handling.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 13211b1 and 98d5938.

Files selected for processing (8)

build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1 hunks)
build/charts/yorkie-cluster/values.yaml (1 hunks)
client/client.go (2 hunks)
server/clients/clients.go (3 hunks)
server/rpc/server.go (1 hunks)
server/rpc/yorkie_server.go (3 hunks)
server/server.go (2 hunks)
test/integration/presence_test.go (1 hunks)

Files skipped from review due to trivial changes (1)

build/charts/yorkie-cluster/values.yaml

Additional comments not posted (15)

build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1)

49-51: LGTM!

The addition of the GATEWAY_HOST environment variable enhances the container's configurability by allowing it to dynamically reference the gateway service based on the provided Helm values.

server/rpc/server.go (1)

67-67: LGTM!

The inclusion of the conf parameter in the NewServer function enhances the flexibility and context-awareness of the service handler's setup by allowing the server's initialization logic to utilize the provided configuration settings.

server/server.go (2)

153-156: LGTM!

The change in the DeactivateClient method enhances the interaction with the backend and RPC mechanisms, ensuring that the client deactivation process is more comprehensive and context-aware.

175-175: LGTM!

The change in the RegisterHousekeepingTasks method improves the integration with remote procedure calls, enhancing the control flow and functionality related to housekeeping tasks.

server/clients/clients.go (4)

23-24: LGTM!

The import changes are appropriate for the new functionality.

155-158: LGTM!

The isEmptyCtx function correctly identifies an empty context.

160-167: LGTM!

The getAuthToken function correctly retrieves the authorization token from the context.

55-134: LGTM! But verify the function usage in the codebase.

The changes to the Deactivate function improve the robustness of the client deactivation process. Ensure that all function calls to Deactivate match the new signature.

test/integration/presence_test.go (1)

470-494: LGTM!

The new test case correctly verifies the presence cleanup during client deactivation by the server.

server/rpc/yorkie_server.go (4)

44-44: LGTM!

The addition of the conf field enhances the flexibility and maintainability of the yorkieServer struct.

48-52: LGTM!

The changes to the newYorkieServer constructor ensure that the conf field is correctly initialized.

99-102: LGTM!

The changes to the DeactivateClient method improve the method's interaction with the database.

662-664: LGTM!

The rpcAddr method enhances the server's configurability by constructing the server's address using the port specified in the conf field.
client/client.go (2)
791-796: LGTM! But verify the method usage in the codebase.

The PretendActivate method sets the client as activated without actual activation. The code changes are approved.

However, ensure that this method is used appropriately within the codebase.

Verification successful

The PretendActivate method is used appropriately in the codebase.

The method is used in server/clients/clients.go as part of the client setup or teardown process, and there are no indications of misuse.

server/clients/clients.go: cli.PretendActivate(actorID)
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the `PretendActivate` method in the codebase.

# Test: Search for the method usage. Expect: Relevant usage in the codebase.
rg --type go 'PretendActivate'
Length of output: 253

Script:
#!/bin/bash
# Description: Extract the context around the usage of the `PretendActivate` method in the codebase.

# Test: Get lines around the usage of `PretendActivate` in `server/clients/clients.go`.
rg --type go -A 10 -B 10 'PretendActivate' server/clients/clients.go
Length of output: 612
780-789: LGTM! But verify the method usage in the codebase.

The PretendAttach method sets a document as attached without actual activation. The code changes are approved.

However, ensure that this method is used appropriately within the codebase.

Verification successful

The PretendAttach method is used appropriately within the codebase.

The method is part of a sequence that builds a document and then detaches it, which aligns with its intended purpose. The surrounding code handles errors properly, confirming its correct usage.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the `PretendAttach` method in the codebase.

# Test: Search for the method usage. Expect: Relevant usage in the codebase.
rg --type go 'PretendAttach'
Length of output: 293

Script:
#!/bin/bash
# Extract the relevant lines around the usage of `PretendAttach` in `server/clients/clients.go`
rg --type go 'PretendAttach' -A 10 -B 10 server/clients/clients.go
Length of output: 509

hackerwins

Thanks for your contribution. Overall looks good.

I have a few questions:

A. Configuration and Management Consistency Issue

The rpcAddr is injected via a flag, then managed through config, and finally injected as an argument to the function where it's used. In contrast, the gateway address is injected via an environment variable. There appears to be a lack of consistency in configuration management and usage. Is there a good solution for this?

B. Reason for Authentication Token

When the Housekeeping node requests a Detach operation from the cluster, it uses an Authorization token. Is there any reason for this?

C. Detach Delegation

The Yorkie cluster implements Consistent Hashing using Istio, with specific nodes responsible for specific Documents. The node running Housekeeping needs to delegate the Detach operation to the node responsible for the Document. To achieve this, the Housekeeping node uses the Gateway address to call Detach, thus distributing the load.

@krapie

I'm curious about your opinion on whether this method is appropriate.

raararaara · 2024-08-07T09:14:03Z

@hackerwins

The answer to question B is:

During the deactivate process, the VerifyAccess() was performed, and a test case that failed was found during this process, such as authorization webhook test.

For the remaining questions, I look forward to hearing ideas from others.

coderabbitai

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)

server/backend/config.go (1)

85-86: Document the GatewayAddr field.

The GatewayAddr field enhances configuration flexibility. Ensure that this field is documented in the configuration files to guide users on its usage.
cmd/yorkie/server.go (1)
348-353: Integration of backend-gateway-addr flag appears incomplete.

The backend-gateway-addr flag is defined but not used elsewhere in the codebase, indicating it might not be integrated into the server initialization process. Please ensure that it is utilized appropriately within the server setup to enhance configurability as intended.

Check the server initialization logic to integrate conf.Backend.GatewayAddr.

Analysis chain

Addition of backend-gateway-addr flag enhances configurability.

The new command-line flag backend-gateway-addr allows users to specify the gateway address, improving server configurability.

Ensure that this new flag is correctly integrated into the server initialization process.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the integration of the `backend-gateway-addr` flag in the server initialization process.

# Test: Search for the usage of `Backend.GatewayAddr` in the codebase. Expect: Correct integration in server initialization.
rg --type go 'Backend.GatewayAddr'
Length of output: 84

Script:
#!/bin/bash
# Description: Search for the usage of `conf.Backend.GatewayAddr` in the codebase to verify its integration.

# Test: Find all references to `conf.Backend.GatewayAddr`.
rg --type go 'conf.Backend.GatewayAddr'
Length of output: 89

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 98d5938 and a1d8ed1.

Files selected for processing (8)

build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1 hunks)
cmd/yorkie/server.go (1 hunks)
server/backend/config.go (1 hunks)
server/clients/clients.go (3 hunks)
server/clients/housekeeping.go (2 hunks)
server/config.go (1 hunks)
server/rpc/yorkie_server.go (3 hunks)
server/server.go (2 hunks)

Files skipped from review as they are similar to previous changes (3)

build/charts/yorkie-cluster/templates/yorkie/deployment.yaml
server/rpc/yorkie_server.go
server/server.go

Additional comments not posted (4)

server/clients/housekeeping.go (1)
41-41: Ensure gatewayAddr is correctly configured.

The addition of the gatewayAddr parameter to DeactivateInactives enhances flexibility. Verify that the gatewayAddr is correctly configured and passed from the caller to ensure proper deactivation.

Verification successful

gatewayAddr is correctly configured and passed.

The gatewayAddr parameter is dynamically configured using be.Config.FetchGatewayAddr(r.RPCAddr()) and passed correctly to the DeactivateInactives function. This ensures that the address is obtained from the appropriate runtime context. No further action is needed.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of `DeactivateInactives` to ensure `gatewayAddr` is correctly configured.

# Test: Search for the function usage. Expect: Correct configuration and passing of `gatewayAddr`.
rg --type go -A 5 $'DeactivateInactives'
Length of output: 838

Script:
#!/bin/bash
# Description: Search for the complete call to `DeactivateInactives` in `server/server.go` to verify `gatewayAddr`.

# Test: Extract the complete function call to ensure all parameters, including `gatewayAddr`, are passed correctly.
rg --type go -A 10 'DeactivateInactives' server/server.go
Length of output: 336
server/clients/clients.go (1)
54-122: Verify document detachment logic and error handling.

The restructuring of the Deactivate function ensures documents are detached before deactivation, which is a good improvement. Verify that all possible errors are handled appropriately, especially during the detachment process, to prevent inconsistencies.

Verification successful

Document detachment logic and error handling are robust.

The Deactivate function includes comprehensive error handling for each step of the document detachment and client deactivation process. This ensures that any errors are appropriately surfaced and can be managed, preventing inconsistencies. No further issues were found.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the logic and error handling in the `Deactivate` function.

# Test: Search for the function implementation. Expect: Proper error handling during detachment.
ast-grep --lang go --pattern $'func Deactivate(ctx context.Context, be *backend.Backend, refKey types.ClientRefKey, gatewayAddr string) ($_, $_) {
  $$$
  if err := cli.Detach(ctx, doc); err != nil {
    $$$
  }
  $$$
}'
Length of output: 221

Script:
#!/bin/bash
# Search for functions related to client deactivation in the server/clients/clients.go file
rg 'func.*Deactivate' server/clients/clients.go -A 20
Length of output: 849

Script:
#!/bin/bash
# Search for the complete implementation of the Deactivate function to review error handling.
rg 'func Deactivate' server/clients/clients.go -A 50
Length of output: 1554

Script:
#!/bin/bash
# Continue searching for the complete implementation of the Deactivate function to review error handling.
rg 'func Deactivate' server/clients/clients.go -A 100
Length of output: 2629
server/backend/config.go (1)
89-95: Ensure consistent use of FetchGatewayAddr.

The FetchGatewayAddr method provides a sensible default for the gateway address. Verify that it is used consistently throughout the codebase where the gateway address is needed.

Verification successful

Consistent usage of FetchGatewayAddr confirmed.

The FetchGatewayAddr method is consistently used across the codebase in relevant files, ensuring that the gateway address is obtained correctly where needed.

Locations of usage:

server/server.go

server/rpc/yorkie_server.go
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of `FetchGatewayAddr` to ensure consistent application.

# Test: Search for the function usage. Expect: Consistent application in the codebase.
rg --type go -A 5 $'FetchGatewayAddr'
Length of output: 1221
server/config.go (1)

71-72: Addition of DefaultGatewayAddr is consistent.

The new constant DefaultGatewayAddr is added consistently with other default configuration values, enhancing configurability.

krapie · 2024-08-09T11:03:26Z

@hackerwins I think using yorkie gateway to delegate detach is a good idea, considering that the housekeeping server will send only necessary request to the server to delegate. ~~I'm wondering what will happen when detach is not performed on other documents that user was attached, and then deactivation occurs?~~

raararaara · 2024-08-14T06:59:18Z

@krapie Could you please explain in more detail what you are curious about?

Are you referring to a situation where document detach fails and only client deactivate occured during the process of client deactivation, specifically clients.deactivate()?

…ring housekeeping

coderabbitai

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)

server/rpc/auth/auth.go (1)
50-54: Security Implications of Bypassing Access Checks with Server Token

The use of ServerToken to bypass access checks in auth.go can introduce security vulnerabilities if not managed properly. Ensure that:

The ServerToken is set to a secure, unique value in production environments.

The default token ("default-token") is not used in production.

Proper documentation and guidelines are in place for managing the ServerToken.

server/rpc/auth/auth.go: Bypass logic using ServerToken.

server/config.go: Default token assignment.

Analysis chain

Verify the implications of bypassing access checks with the server token.

The new conditional check allows bypassing further access verification if the Authorization matches the ServerToken. Ensure that this temporary measure does not introduce security vulnerabilities or bypass critical access controls.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of the server token in access control logic.

# Test: Search for the usage of `ServerToken` in the codebase. Expect: Controlled and secure usage.
rg --type go 'ServerToken'
Length of output: 418

Script:
#!/bin/bash
# Search for all occurrences of ServerToken assignment and usage to understand its implications.
rg --type go 'ServerToken ='
rg --type go 'ServerToken'
Length of output: 617

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a1d8ed1 and 6619673.

Files selected for processing (6)

build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1 hunks)
server/backend/config.go (1 hunks)
server/backend/housekeeping/housekeeping.go (2 hunks)
server/config.go (2 hunks)
server/rpc/auth/auth.go (1 hunks)
test/helper/helper.go (1 hunks)

Files skipped from review as they are similar to previous changes (3)

build/charts/yorkie-cluster/templates/yorkie/deployment.yaml
server/backend/config.go
server/config.go

Additional comments not posted (2)

server/backend/housekeeping/housekeeping.go (1)
60-63: Ensure secure handling of the server token environment variable.

The Authorization field is populated using the YORKIE_SERVER_TOKEN environment variable. Verify that this environment variable is securely managed and not exposed inappropriately.

Verification successful

Secure Handling of YORKIE_SERVER_TOKEN Verified

The YORKIE_SERVER_TOKEN environment variable is used appropriately in the codebase without any signs of exposure or insecure handling. It is set in a Kubernetes deployment YAML file, which is a standard practice for managing sensitive configurations in a secure manner. No further actions are needed.

server/backend/housekeeping/housekeeping.go: Used to set the Authorization field.

build/charts/yorkie-cluster/templates/yorkie/deployment.yaml: Defined as an environment variable.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the secure handling of the `YORKIE_SERVER_TOKEN` environment variable.

# Test: Search for the usage of `YORKIE_SERVER_TOKEN` in the codebase. Expect: Secure and controlled usage.
rg --type go 'YORKIE_SERVER_TOKEN'
Length of output: 133

Script:
#!/bin/bash
# Search for any potential exposure of YORKIE_SERVER_TOKEN in the codebase
rg 'YORKIE_SERVER_TOKEN' --type go --type toml --type yaml --type json --type sh
Length of output: 276
test/helper/helper.go (1)

258-258: Verify the impact of adding ServerToken on existing tests and security.

The addition of the ServerToken field in the test configuration may affect existing tests. Ensure that this change does not introduce any security vulnerabilities or break existing test cases.

raararaara · 2024-08-14T09:13:57Z

I have identified an issue related to Authorization as pointed out by @hackerwins . Specifically, the issues are as follows:

For clients with a token value, the verifyAccess process is required during deactivate or detach.
- When receiving a deactivate or detach request from an external source, the client's token value is passed along with the request.
- However, in cases where a client is temporarily created during the server (Housekeeping) process, there is no way to know this token value.

To address this issue, I'm considering the following two actions:

Issuing a Server-Specific Token:
We plan to generate a shared token for all servers, allowing us to skip the verifyAccess step for server-specific tokens. We are considering methods such as injecting this token through the defaultFlag or a DevOps manifest.
Defining a systemService:
Currently, only rpcService and adminService are defined. We propose defining a new systemService to separate the logic for inter-server communication from existing services.

Given the additional work required, I will mark this PR as a draft.

And of course, any feedback or comments are welcome!

krapie · 2024-08-14T15:22:23Z

@raararaara My apologizes, I was asking the wrong question.

This reverts commit 0aae9a0.

raararaara added 2 commits July 19, 2024 18:03

Modify clients.Deactivate to handle presence clear

5d48edb

Resolve conflict

fb631fa

hackerwins added 2 commits July 22, 2024 11:29

Fix invalid argument passing

90cfe15

Fix lint warning

e496ad2

github-actions bot reviewed Jul 22, 2024

View reviewed changes

raararaara added 9 commits July 22, 2024 14:34

Add related test

a87a9f1

Add logic to prevent duplicate presence clear change

1da3ec3

Simplify presenceChange clear condition

525d4c4

Add test related to duplicate presence clear

28b0db5

Lint

1c5b05c

Fix test to return correct result

e0b9fce

Add additional comments

597b867

Cleanup code

e39b542

Cleanup code

74bca05

raararaara marked this pull request as ready for review July 23, 2024 09:25

raararaara requested a review from hackerwins July 23, 2024 09:26

coderabbitai bot reviewed Jul 23, 2024

View reviewed changes

Revise the deactivation logic

3409bf1

coderabbitai bot reviewed Jul 23, 2024

View reviewed changes

Revert queries and test related to DeactivateClient

e4ded21

coderabbitai bot reviewed Jul 24, 2024

View reviewed changes

Modify clients.Deactivate to load client within the method

13211b1

coderabbitai bot reviewed Jul 26, 2024

View reviewed changes

server/backend/database/client_info.go Outdated Show resolved Hide resolved

server/backend/database/client_info.go Outdated Show resolved Hide resolved

server/backend/database/client_info.go Outdated Show resolved Hide resolved

raararaara marked this pull request as draft July 26, 2024 06:10

raararaara added 4 commits July 26, 2024 17:12

Fix rpcAddr to cover other testcases

cc14a6c

Lint

ab278e7

Fix ToClient to allow token value to be received when creating clie…

17de638

…nt from clientInfo

Lint

532f87d

raararaara added 5 commits August 1, 2024 19:59

Refactored clients.Deactivate to use client.Detach

5ad0b89

Add gateway host to Yorkie deployment

6034b7d

Lint

5733c0e

Modified client creation to be performed at clients.Deactivate

c2da835

Refactored comment to clarify APIhost determination logic based on en…

98d5938

…vironment

raararaara marked this pull request as ready for review August 6, 2024 09:01

coderabbitai bot reviewed Aug 6, 2024

View reviewed changes

hackerwins reviewed Aug 7, 2024

View reviewed changes

hackerwins force-pushed the main branch 2 times, most recently from fbc6098 to 7d65a57 Compare August 9, 2024 02:00

hackerwins requested a review from krapie August 9, 2024 02:02

Move backend-gateway-addr to flag

a1d8ed1

coderabbitai bot reviewed Aug 9, 2024

View reviewed changes

Added server token to ensure authorization for client deactivation du…

6619673

…ring housekeeping

coderabbitai bot reviewed Aug 14, 2024

View reviewed changes

raararaara marked this pull request as draft August 14, 2024 09:14

raararaara added 2 commits August 19, 2024 16:22

Add systemService to handle perform background deactivation

0aae9a0

Revert "Add systemService to handle perform background deactivation"

3c5f5e1

This reverts commit 0aae9a0.

hackerwins force-pushed the main branch from 8eb4284 to b77c25a Compare September 5, 2024 08:36

raararaara added 6 commits September 24, 2024 18:12

Add system server to handle perform housekeeping deactivation

4ae640a

Fix lint

9728100

Fix lint proto

c24411b

Fix lint proto

ab77581

Modify buf.yaml temporarily to allow buf lint for CI test verification

60425b7

Introduce system client

1ea7a8d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear presence when housekeeping process #936

Clear presence when housekeeping process #936

raararaara commented Jul 22, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 22, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

github-actions bot left a comment •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

hackerwins left a comment

raararaara commented Aug 7, 2024

coderabbitai bot left a comment

krapie commented Aug 9, 2024 •

edited

Loading

raararaara commented Aug 14, 2024

coderabbitai bot left a comment

raararaara commented Aug 14, 2024

krapie commented Aug 14, 2024

Clear presence when housekeeping process #936

Are you sure you want to change the base?

Clear presence when housekeeping process #936

Conversation

raararaara commented Jul 22, 2024 • edited by coderabbitai bot Loading

What this PR does / why we need it?

Any background context you want to provide?

What are the relevant tickets?

Summary by CodeRabbit

Summary by CodeRabbit

coderabbitai bot commented Jul 22, 2024 • edited Loading

Review skipped

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Go Benchmark

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

hackerwins left a comment

Choose a reason for hiding this comment

A. Configuration and Management Consistency Issue

B. Reason for Authentication Token

C. Detach Delegation

raararaara commented Aug 7, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

krapie commented Aug 9, 2024 • edited Loading

raararaara commented Aug 14, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

raararaara commented Aug 14, 2024

krapie commented Aug 14, 2024

raararaara commented Jul 22, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 22, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

github-actions bot left a comment •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

krapie commented Aug 9, 2024 •

edited

Loading