Detach documents when client is deactivated #1036
Conversation
WalkthroughThe pull request introduces new OpenAPI specification files for the Yorkie API, version 3.1.0, detailing metadata, server environments, and new endpoints for detaching documents. Protocol Buffers files define the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (12)
api/yorkie/v1/system.proto (2)
1-24: Consider adjusting the directory structure to match the package name.The package name
yorkie.v1is appropriate, but the static analysis tool suggests that files with this package should be within a directory "yorkie/v1" relative to the root, rather than "api/yorkie/v1".Consider one of the following options:
- Move this file to a "yorkie/v1" directory if it aligns with your project structure.
- If the current structure is intentional, you may want to configure the static analysis tool to ignore this warning for your specific project setup.
🧰 Tools
🪛 buf
18-18: Files with package "yorkie.v1" must be within a directory "yorkie/v1" relative to root but were in directory "api/yorkie/v1".
(PACKAGE_DIRECTORY_MATCH)
25-28: Consider renaming request and response types to follow standard conventions.The static analysis tool suggests that the RPC request and response types should follow a standard naming convention.
Consider renaming the types as follows:
DetachDocumentRequestBySystemtoDetachDocumentRequestorSystemServiceDetachDocumentRequestDetachDocumentResponseBySystemtoDetachDocumentResponseorSystemServiceDetachDocumentResponseThis change would improve consistency with common protobuf naming conventions and make the API more intuitive for developers familiar with these standards.
🧰 Tools
🪛 buf
27-27: RPC request type "DetachDocumentRequestBySystem" should be named "DetachDocumentRequest" or "SystemServiceDetachDocumentRequest".
(RPC_REQUEST_STANDARD_NAME)
27-27: RPC response type "DetachDocumentResponseBySystem" should be named "DetachDocumentResponse" or "SystemServiceDetachDocumentResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: build
[failure] 27-27:
RPC request type "DetachDocumentRequestBySystem" should be named "DetachDocumentRequest" or "SystemServiceDetachDocumentRequest".
[failure] 27-27:
RPC response type "DetachDocumentResponseBySystem" should be named "DetachDocumentResponse" or "SystemServiceDetachDocumentResponse".api/docs/yorkie/v1/system.openapi.yaml (2)
12-24: Consider adding a description for the DetachDocument endpoint.The path structure and references are well-defined. However, the description field for the DetachDocument endpoint is empty. Adding a brief description would improve the API documentation and help users understand the purpose of this endpoint.
Consider adding a description like:
description: Detaches a document from a client in the specified project.
105-106: Address TODO comment for future improvement.There's a TODO comment suggesting a potential improvement to replace certain fields with more structured types.
Would you like me to create a GitHub issue to track this improvement? The issue could be titled "Refactor DetachDocumentRequestBySystem to use structured types" and include the following description:
"Consider replacing the fields in DetachDocumentRequestBySystem with types.Project, types.Client, and types.DocumentSummary for improved structure and type safety."server/server.go (1)
Line range hint
153-157: LGTM with a minor suggestion for error handlingThe changes to the
DeactivateClientmethod look good. The addition of theprojectparameter and its usage in theclients.Deactivatefunction call improves the flexibility of the method by allowing project-specific client deactivation. This aligns well with the PR objective of detaching documents when a client is deactivated.The fallback to the default project maintains backwards compatibility, which is a good practice.
Consider wrapping the error returned from
clients.Deactivatewith additional context:- _, err = clients.Deactivate(ctx, r.backend.DB, project, types.ClientRefKey{ + _, err = clients.Deactivate(ctx, r.backend.DB, project, types.ClientRefKey{ ProjectID: project.ID, ClientID: types.IDFromActorID(c1.ID()), }) - return err + if err != nil { + return fmt.Errorf("failed to deactivate client: %w", err) + } + return nilThis will provide more context if an error occurs during client deactivation.
pkg/document/internal_document.go (1)
375-381: LGTM! Consider adding a brief comment for clarity.The
ToDocumentmethod looks good and serves its purpose well. It correctly converts anInternalDocumentto aDocumenttype.Consider adding a brief comment to explain the purpose of this method and any important details about the conversion process. For example:
// ToDocument converts the InternalDocument to a Document. // Note: The returned Document shares the same underlying data as the InternalDocument. func (d *InternalDocument) ToDocument() *Document { // ... (existing implementation) }This additional comment would help other developers understand the method's behavior and any potential side effects.
pkg/document/document.go (1)
457-459: Add documentation for thesetInternalDocmethod.The newly added
setInternalDocmethod lacks documentation. To improve code maintainability and help other developers understand its purpose, please add a comment explaining the method's functionality, when it should be used, and any potential side effects.Consider adding a comment like this:
// setInternalDoc replaces the internal document of the Document. // This method should be used with caution as it directly modifies // the internal state without any validation.server/rpc/yorkie_server.go (1)
97-100: LGTM! Consider enhancing error handling.The addition of the
projectparameter to theclients.Deactivatefunction call is a good improvement. It aligns with the PR objective of detaching documents when a client is deactivated by providing the necessary project context.Consider wrapping the
clients.Deactivatecall in a more descriptive error:- _, err = clients.Deactivate(ctx, s.backend.DB, project, types.ClientRefKey{ + _, err = clients.Deactivate(ctx, s.backend.DB, project, types.ClientRefKey{ ProjectID: project.ID, ClientID: types.IDFromActorID(actorID), }) + if err != nil { + return nil, fmt.Errorf("failed to deactivate client: %w", err) + }This change would provide more context in case of an error, making debugging easier.
server/clients/housekeeping.go (2)
91-95: EnsureCandidatePairaligns with Go naming conventionsThe fields in
CandidatePairare unexported (start with lowercase letters). IfCandidatePairis intended to be used outside its package, consider exporting the fields by capitalizing them. If it's only for internal use, this is acceptable.
126-129: Simplify determination oftopProjectIDThe logic for setting
topProjectIDcan be simplified for readability.Consider the following refactored code:
var topProjectID types.ID = database.DefaultProjectID if len(projectInfos) == projectFetchSize { topProjectID = projectInfos[len(projectInfos)-1].ID }system/client.go (2)
154-156: Use standard Go comment style for function documentationThe comment for
withShardKeyuses block comment syntax/** ... */, which is unconventional in Go. Go documentation comments should start with the function name and use line comments (//).Apply this diff to fix the comment style:
-/** -* withShardKey returns a context with the given shard key in metadata. - */ +// withShardKey returns a request with the given shard key added to the headers.
95-107: Consider renaming functions to avoid confusion betweenDialmethodsCurrently, both the constructor function and the method on
Clientare namedDial. This may lead to confusion when reading the code.Consider renaming the constructor function
Dial(lines 95-107) toDialClientorNewClientfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/yorkie/v1/system.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
- api/docs/yorkie/v1/admin.openapi.yaml (56 hunks)
- api/docs/yorkie/v1/resources.openapi.yaml (65 hunks)
- api/docs/yorkie/v1/system.openapi.yaml (1 hunks)
- api/docs/yorkie/v1/yorkie.openapi.yaml (50 hunks)
- api/yorkie/v1/system.proto (1 hunks)
- api/yorkie/v1/v1connect/system.connect.go (1 hunks)
- api/yorkie/v1/yorkie.proto (1 hunks)
- pkg/document/document.go (1 hunks)
- pkg/document/internal_document.go (1 hunks)
- server/backend/database/client_info.go (1 hunks)
- server/backend/database/mongo/client.go (1 hunks)
- server/clients/clients.go (2 hunks)
- server/clients/housekeeping.go (2 hunks)
- server/rpc/server.go (1 hunks)
- server/rpc/system_server.go (1 hunks)
- server/rpc/yorkie_server.go (1 hunks)
- server/server.go (1 hunks)
- system/client.go (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- api/docs/yorkie/v1/admin.openapi.yaml
- api/docs/yorkie/v1/resources.openapi.yaml
- api/docs/yorkie/v1/yorkie.openapi.yaml
- api/yorkie/v1/yorkie.proto
🧰 Additional context used
🪛 buf
api/yorkie/v1/system.proto
18-18: Files with package "yorkie.v1" must be within a directory "yorkie/v1" relative to root but were in directory "api/yorkie/v1".
(PACKAGE_DIRECTORY_MATCH)
27-27: RPC request type "DetachDocumentRequestBySystem" should be named "DetachDocumentRequest" or "SystemServiceDetachDocumentRequest".
(RPC_REQUEST_STANDARD_NAME)
27-27: RPC response type "DetachDocumentResponseBySystem" should be named "DetachDocumentResponse" or "SystemServiceDetachDocumentResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: build
api/yorkie/v1/system.proto
[failure] 27-27:
RPC request type "DetachDocumentRequestBySystem" should be named "DetachDocumentRequest" or "SystemServiceDetachDocumentRequest".
[failure] 27-27:
RPC response type "DetachDocumentResponseBySystem" should be named "DetachDocumentResponse" or "SystemServiceDetachDocumentResponse".
🔇 Additional comments (19)
api/yorkie/v1/system.proto (1)
30-38: Review TODO comment and consider response message structure.
- The TODO comment suggests replacing simple string fields with more complex types. This could indeed provide more robust type checking and potentially more functionality.
Consider creating a follow-up task to implement this suggestion, as it could improve the API's type safety and expressiveness.
- The
DetachDocumentResponseBySystemmessage is currently empty.Is this intentional? If no response data is needed, this is fine. However, if you anticipate adding fields in the future or if there's any useful information that could be returned (e.g., a success flag or timestamp), consider adding them now to future-proof the API.
✅ Verification successful
Verified: TODO can be addressed using existing types, and the response message structure is appropriate.
- The types
Project,Client, andDocumentSummaryare well-defined and widely used, supporting the proposed replacement of string fields.- The
DetachDocumentResponseBySystemmessage is intentionally empty with no fields expected by any usage, confirming its correctness.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any usages of DetachDocumentResponseBySystem that expect fields rg "DetachDocumentResponseBySystem" --type go -C 5Length of output: 9913
Script:
#!/bin/bash # Find definitions of types.Project, types.Client, and types.DocumentSummary rg "type\s+Project\s+struct" --type go rg "type\s+Client\s+struct" --type go rg "type\s+DocumentSummary\s+struct" --type go # Check usages of types.Project, types.Client, and types.DocumentSummary in the codebase rg "types\.Project" --type go rg "types\.Client" --type go rg "types\.DocumentSummary" --type goLength of output: 11404
api/docs/yorkie/v1/system.openapi.yaml (5)
1-6: LGTM: API metadata is well-defined.The OpenAPI version and API information are correctly specified. The use of OpenAPI 3.1.0 is appropriate, and the API metadata (title, description, and version) is concise and informative.
7-11: LGTM: Server environments are well-defined.The server definitions for both production and local environments are clear and appropriate. This setup facilitates easy switching between development and production environments.
25-115: LGTM: Components section is well-structured.The components section is comprehensive and follows OpenAPI standards. The schemas for error handling and the DetachDocument operation are well-defined, providing clear structure for requests and responses.
116-120: LGTM: Security scheme is well-defined.The API key authentication scheme is appropriately defined. Using the Authorization header for the API key follows common security practices for API authentication.
123-125: LGTM: Tag is well-defined with a clear description.The tag for yorkie.v1.SystemService is appropriately defined with a clear and concise description. This provides useful context for the API's structure and purpose.
server/rpc/server.go (1)
76-76: Approve the addition of SystemService handler with suggestions.The addition of the SystemService handler is consistent with the existing architecture and enhances the server's functionality. However, there are a couple of points to consider:
- Unlike the YorkieService and AdminService handlers, the SystemService handler doesn't use the
opts...parameter. Is this intentional? If not, consider adding it for consistency:-mux.Handle(v1connect.NewSystemServiceHandler(newSystemServer(be))) +mux.Handle(v1connect.NewSystemServiceHandler(newSystemServer(be), opts...))
- The
newSystemServerfunction is not visible in this file. Please ensure it exists and is properly implemented in the appropriate package.To verify the existence and implementation of the
newSystemServerfunction, please run the following script:✅ Verification successful
Verified: SystemService handler addition
The
newSystemServerfunction and thesystemServerstruct are properly implemented and integrated. The addition of the SystemService handler is consistent with the existing architecture.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and implementation of newSystemServer function # Test: Search for the newSystemServer function definition rg --type go -g '!*_test.go' 'func newSystemServer\(' # Test: Check if the SystemServer type is defined rg --type go -g '!*_test.go' 'type SystemServer struct'Length of output: 204
Script:
#!/bin/bash # Description: Verify the existence and implementation of systemServer struct # Test: Search for the systemServer struct definition rg --type go -g '!*_test.go' 'type systemServer struct'Length of output: 112
server/backend/database/client_info.go (1)
203-212: Add a comment explaining the method's purpose.The implementation of the
ServerSeqmethod looks good. However, it would be beneficial to add a comment explaining the purpose of this method and when it should be used. This will improve code readability and maintainability.Consider adding a comment like this:
// ServerSeq returns the server sequence of the given document. // It returns an error if the document is not found in the client's document map.LGTM with a minor suggestion.
The implementation is correct and consistent with the existing codebase. As a minor suggestion, consider checking if the document is in the "attached" state before returning the
ServerSeq. This could prevent potential issues if the sequence is accessed for detached or removed documents.Let's verify the usage of this new method in the codebase:
✅ Verification successful
The
ServerSeqmethod is utilized appropriately across the codebase.The usage of the
ServerSeqmethod in various components confirms that it integrates well and functions as intended. No issues were identified during the verification process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new ServerSeq method # Search for ServerSeq method calls echo "Searching for ServerSeq method calls:" rg --type go "ServerSeq\(" --glob '!server/backend/database/client_info.go' # Search for potential places where ServerSeq might be useful echo "Searching for potential places where ServerSeq might be used:" rg --type go "ServerSeq|server_seq" --glob '!server/backend/database/client_info.go'Length of output: 32764
pkg/document/document.go (1)
457-459: Verify the intended use and safety ofsetInternalDoc.The
setInternalDocmethod allows direct modification of the internal document without any apparent validation or consistency checks. This could potentially lead to inconsistent state if not used carefully.Please confirm:
- Is this method intended for internal use only (e.g., during synchronization or recovery)?
- Are there any scenarios where this method could lead to data inconsistency?
- Should any validation be added to ensure the new internal document is in a valid state?
Consider adding safeguards or validation if necessary to prevent potential issues.
server/rpc/system_server.go (7)
1-15: License header is correctly includedThe Apache License header is properly added to the new file.
19-37: Imports are organized appropriatelyThe necessary packages are imported, and there are no unused imports.
39-41: Definition ofsystemServerThe
systemServerstruct is correctly defined with a reference tobackend.Backend.
43-48: ConstructornewSystemServeris implemented properlyThe function
newSystemServerinitializes a newsystemServerinstance with the provided backend.
79-83: Handle locker unlock errors appropriatelyIn the deferred function, when unlocking the locker, you log the error if it occurs. While logging is helpful, consider if additional actions are necessary to handle such errors, depending on how critical the unlock failure might be for your application.
121-134: Verify the correctness ofPushPullOptionsConfirm that setting the
Modetotypes.SyncModePushPulland theStatustodocument.StatusDetachedaligns with the intended behavior for detaching documents. Ensure that these options correctly reflect the desired state changes in the document's lifecycle.
136-137: Response creation is appropriateThe function correctly returns a new response indicating the successful detachment of the document.
api/yorkie/v1/v1connect/system.connect.go (3)
105-112: Review HTTP handler paths for correct routing.In the
NewSystemServiceHandlerfunction, the HTTP handler routes requests based on URL paths:Please verify that:
- The base path
"/yorkie.v1.SystemService/"correctly matches incoming requests.- There are no conflicts with other service routes.
- All expected procedures are properly handled in the switch statement.
This ensures that requests are routed to the correct handlers without unintended collisions.
85-87:⚠️ Potential issueEnsure the
DetachDocumentmethod is correctly implemented and tested.The
DetachDocumentmethod has been added to theSystemServiceClient. Please verify that:
- The method is properly implemented on both client and server sides.
- All logic inside the method aligns with the intended functionality.
- Comprehensive unit tests and integration tests are added to cover this new method.
16-16: Verify if generated code should be committed to version control.Line 16 indicates that this file is generated code:
Generally, it's recommended to exclude generated code from version control to prevent merge conflicts and ensure consistency by regenerating as needed. Confirm whether including this generated file aligns with the project's version control policies.
To check for other generated files committed to the repository, you can run:
cdf4b69 to
31fc46d
Compare
There was a problem hiding this comment.
Go Benchmark
| Benchmark suite | Current: f89f6a4 | Previous: cdf4b69 | Ratio |
|---|---|---|---|
BenchmarkDocument/constructor_test |
1626 ns/op 1337 B/op 24 allocs/op |
1500 ns/op 1337 B/op 24 allocs/op |
1.08 |
BenchmarkDocument/constructor_test - ns/op |
1626 ns/op |
1500 ns/op |
1.08 |
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 |
956.5 ns/op 1305 B/op 22 allocs/op |
957.7 ns/op 1305 B/op 22 allocs/op |
1.00 |
BenchmarkDocument/status_test - ns/op |
956.5 ns/op |
957.7 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 |
7616 ns/op 7273 B/op 132 allocs/op |
7693 ns/op 7273 B/op 132 allocs/op |
0.99 |
BenchmarkDocument/equals_test - ns/op |
7616 ns/op |
7693 ns/op |
0.99 |
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 |
16811 ns/op 12138 B/op 262 allocs/op |
16915 ns/op 12139 B/op 262 allocs/op |
0.99 |
BenchmarkDocument/nested_update_test - ns/op |
16811 ns/op |
16915 ns/op |
0.99 |
BenchmarkDocument/nested_update_test - B/op |
12138 B/op |
12139 B/op |
1.00 |
BenchmarkDocument/nested_update_test - allocs/op |
262 allocs/op |
262 allocs/op |
1 |
BenchmarkDocument/delete_test |
22578 ns/op 15363 B/op 341 allocs/op |
22730 ns/op 15364 B/op 341 allocs/op |
0.99 |
BenchmarkDocument/delete_test - ns/op |
22578 ns/op |
22730 ns/op |
0.99 |
BenchmarkDocument/delete_test - B/op |
15363 B/op |
15364 B/op |
1.00 |
BenchmarkDocument/delete_test - allocs/op |
341 allocs/op |
341 allocs/op |
1 |
BenchmarkDocument/object_test |
8664 ns/op 6817 B/op 120 allocs/op |
9394 ns/op 6817 B/op 120 allocs/op |
0.92 |
BenchmarkDocument/object_test - ns/op |
8664 ns/op |
9394 ns/op |
0.92 |
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 |
29715 ns/op 11946 B/op 276 allocs/op |
30253 ns/op 11947 B/op 276 allocs/op |
0.98 |
BenchmarkDocument/array_test - ns/op |
29715 ns/op |
30253 ns/op |
0.98 |
BenchmarkDocument/array_test - B/op |
11946 B/op |
11947 B/op |
1.00 |
BenchmarkDocument/array_test - allocs/op |
276 allocs/op |
276 allocs/op |
1 |
BenchmarkDocument/text_test |
31366 ns/op 14779 B/op 486 allocs/op |
31709 ns/op 14780 B/op 486 allocs/op |
0.99 |
BenchmarkDocument/text_test - ns/op |
31366 ns/op |
31709 ns/op |
0.99 |
BenchmarkDocument/text_test - B/op |
14779 B/op |
14780 B/op |
1.00 |
BenchmarkDocument/text_test - allocs/op |
486 allocs/op |
486 allocs/op |
1 |
BenchmarkDocument/text_composition_test |
29933 ns/op 18462 B/op 502 allocs/op |
30323 ns/op 18460 B/op 502 allocs/op |
0.99 |
BenchmarkDocument/text_composition_test - ns/op |
29933 ns/op |
30323 ns/op |
0.99 |
BenchmarkDocument/text_composition_test - B/op |
18462 B/op |
18460 B/op |
1.00 |
BenchmarkDocument/text_composition_test - allocs/op |
502 allocs/op |
502 allocs/op |
1 |
BenchmarkDocument/rich_text_test |
82404 ns/op 38708 B/op 1165 allocs/op |
84275 ns/op 38708 B/op 1165 allocs/op |
0.98 |
BenchmarkDocument/rich_text_test - ns/op |
82404 ns/op |
84275 ns/op |
0.98 |
BenchmarkDocument/rich_text_test - B/op |
38708 B/op |
38708 B/op |
1 |
BenchmarkDocument/rich_text_test - allocs/op |
1165 allocs/op |
1165 allocs/op |
1 |
BenchmarkDocument/counter_test |
17256 ns/op 10722 B/op 244 allocs/op |
17555 ns/op 10722 B/op 244 allocs/op |
0.98 |
BenchmarkDocument/counter_test - ns/op |
17256 ns/op |
17555 ns/op |
0.98 |
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 |
1302210 ns/op 871967 B/op 17276 allocs/op |
1323658 ns/op 872000 B/op 17275 allocs/op |
0.98 |
BenchmarkDocument/text_edit_gc_100 - ns/op |
1302210 ns/op |
1323658 ns/op |
0.98 |
BenchmarkDocument/text_edit_gc_100 - B/op |
871967 B/op |
872000 B/op |
1.00 |
BenchmarkDocument/text_edit_gc_100 - allocs/op |
17276 allocs/op |
17275 allocs/op |
1.00 |
BenchmarkDocument/text_edit_gc_1000 |
50825918 ns/op 50545582 B/op 186738 allocs/op |
50725465 ns/op 50547210 B/op 186731 allocs/op |
1.00 |
BenchmarkDocument/text_edit_gc_1000 - ns/op |
50825918 ns/op |
50725465 ns/op |
1.00 |
BenchmarkDocument/text_edit_gc_1000 - B/op |
50545582 B/op |
50547210 B/op |
1.00 |
BenchmarkDocument/text_edit_gc_1000 - allocs/op |
186738 allocs/op |
186731 allocs/op |
1.00 |
BenchmarkDocument/text_split_gc_100 |
1920827 ns/op 1588466 B/op 15944 allocs/op |
1962373 ns/op 1588527 B/op 15945 allocs/op |
0.98 |
BenchmarkDocument/text_split_gc_100 - ns/op |
1920827 ns/op |
1962373 ns/op |
0.98 |
BenchmarkDocument/text_split_gc_100 - B/op |
1588466 B/op |
1588527 B/op |
1.00 |
BenchmarkDocument/text_split_gc_100 - allocs/op |
15944 allocs/op |
15945 allocs/op |
1.00 |
BenchmarkDocument/text_split_gc_1000 |
115794976 ns/op 141483556 B/op 186147 allocs/op |
116636715 ns/op 141481421 B/op 186139 allocs/op |
0.99 |
BenchmarkDocument/text_split_gc_1000 - ns/op |
115794976 ns/op |
116636715 ns/op |
0.99 |
BenchmarkDocument/text_split_gc_1000 - B/op |
141483556 B/op |
141481421 B/op |
1.00 |
BenchmarkDocument/text_split_gc_1000 - allocs/op |
186147 allocs/op |
186139 allocs/op |
1.00 |
BenchmarkDocument/text_delete_all_10000 |
16487742 ns/op 10213870 B/op 55683 allocs/op |
17345072 ns/op 10212741 B/op 55681 allocs/op |
0.95 |
BenchmarkDocument/text_delete_all_10000 - ns/op |
16487742 ns/op |
17345072 ns/op |
0.95 |
BenchmarkDocument/text_delete_all_10000 - B/op |
10213870 B/op |
10212741 B/op |
1.00 |
BenchmarkDocument/text_delete_all_10000 - allocs/op |
55683 allocs/op |
55681 allocs/op |
1.00 |
BenchmarkDocument/text_delete_all_100000 |
278606239 ns/op 142972576 B/op 561724 allocs/op |
314986961 ns/op 142962208 B/op 561644 allocs/op |
0.88 |
BenchmarkDocument/text_delete_all_100000 - ns/op |
278606239 ns/op |
314986961 ns/op |
0.88 |
BenchmarkDocument/text_delete_all_100000 - B/op |
142972576 B/op |
142962208 B/op |
1.00 |
BenchmarkDocument/text_delete_all_100000 - allocs/op |
561724 allocs/op |
561644 allocs/op |
1.00 |
BenchmarkDocument/text_100 |
220112 ns/op 120235 B/op 5180 allocs/op |
230518 ns/op 120235 B/op 5180 allocs/op |
0.95 |
BenchmarkDocument/text_100 - ns/op |
220112 ns/op |
230518 ns/op |
0.95 |
BenchmarkDocument/text_100 - B/op |
120235 B/op |
120235 B/op |
1 |
BenchmarkDocument/text_100 - allocs/op |
5180 allocs/op |
5180 allocs/op |
1 |
BenchmarkDocument/text_1000 |
2384193 ns/op 1171023 B/op 51084 allocs/op |
2480969 ns/op 1171025 B/op 51084 allocs/op |
0.96 |
BenchmarkDocument/text_1000 - ns/op |
2384193 ns/op |
2480969 ns/op |
0.96 |
BenchmarkDocument/text_1000 - B/op |
1171023 B/op |
1171025 B/op |
1.00 |
BenchmarkDocument/text_1000 - allocs/op |
51084 allocs/op |
51084 allocs/op |
1 |
BenchmarkDocument/array_1000 |
1203279 ns/op 1091486 B/op 11832 allocs/op |
1272389 ns/op 1091380 B/op 11831 allocs/op |
0.95 |
BenchmarkDocument/array_1000 - ns/op |
1203279 ns/op |
1272389 ns/op |
0.95 |
BenchmarkDocument/array_1000 - B/op |
1091486 B/op |
1091380 B/op |
1.00 |
BenchmarkDocument/array_1000 - allocs/op |
11832 allocs/op |
11831 allocs/op |
1.00 |
BenchmarkDocument/array_10000 |
13101131 ns/op 9799925 B/op 120296 allocs/op |
13806197 ns/op 9800443 B/op 120299 allocs/op |
0.95 |
BenchmarkDocument/array_10000 - ns/op |
13101131 ns/op |
13806197 ns/op |
0.95 |
BenchmarkDocument/array_10000 - B/op |
9799925 B/op |
9800443 B/op |
1.00 |
BenchmarkDocument/array_10000 - allocs/op |
120296 allocs/op |
120299 allocs/op |
1.00 |
BenchmarkDocument/array_gc_100 |
144651 ns/op 132732 B/op 1261 allocs/op |
156238 ns/op 132715 B/op 1260 allocs/op |
0.93 |
BenchmarkDocument/array_gc_100 - ns/op |
144651 ns/op |
156238 ns/op |
0.93 |
BenchmarkDocument/array_gc_100 - B/op |
132732 B/op |
132715 B/op |
1.00 |
BenchmarkDocument/array_gc_100 - allocs/op |
1261 allocs/op |
1260 allocs/op |
1.00 |
BenchmarkDocument/array_gc_1000 |
1379574 ns/op 1159238 B/op 12877 allocs/op |
1472999 ns/op 1159228 B/op 12877 allocs/op |
0.94 |
BenchmarkDocument/array_gc_1000 - ns/op |
1379574 ns/op |
1472999 ns/op |
0.94 |
BenchmarkDocument/array_gc_1000 - B/op |
1159238 B/op |
1159228 B/op |
1.00 |
BenchmarkDocument/array_gc_1000 - allocs/op |
12877 allocs/op |
12877 allocs/op |
1 |
BenchmarkDocument/counter_1000 |
200428 ns/op 193079 B/op 5771 allocs/op |
216084 ns/op 193080 B/op 5771 allocs/op |
0.93 |
BenchmarkDocument/counter_1000 - ns/op |
200428 ns/op |
216084 ns/op |
0.93 |
BenchmarkDocument/counter_1000 - B/op |
193079 B/op |
193080 B/op |
1.00 |
BenchmarkDocument/counter_1000 - allocs/op |
5771 allocs/op |
5771 allocs/op |
1 |
BenchmarkDocument/counter_10000 |
2182622 ns/op 2087982 B/op 59778 allocs/op |
2231338 ns/op 2087999 B/op 59778 allocs/op |
0.98 |
BenchmarkDocument/counter_10000 - ns/op |
2182622 ns/op |
2231338 ns/op |
0.98 |
BenchmarkDocument/counter_10000 - B/op |
2087982 B/op |
2087999 B/op |
1.00 |
BenchmarkDocument/counter_10000 - allocs/op |
59778 allocs/op |
59778 allocs/op |
1 |
BenchmarkDocument/object_1000 |
1369289 ns/op 1428157 B/op 9849 allocs/op |
1463005 ns/op 1428097 B/op 9849 allocs/op |
0.94 |
BenchmarkDocument/object_1000 - ns/op |
1369289 ns/op |
1463005 ns/op |
0.94 |
BenchmarkDocument/object_1000 - B/op |
1428157 B/op |
1428097 B/op |
1.00 |
BenchmarkDocument/object_1000 - allocs/op |
9849 allocs/op |
9849 allocs/op |
1 |
BenchmarkDocument/object_10000 |
15115176 ns/op 12168150 B/op 100566 allocs/op |
15714521 ns/op 12167743 B/op 100567 allocs/op |
0.96 |
BenchmarkDocument/object_10000 - ns/op |
15115176 ns/op |
15714521 ns/op |
0.96 |
BenchmarkDocument/object_10000 - B/op |
12168150 B/op |
12167743 B/op |
1.00 |
BenchmarkDocument/object_10000 - allocs/op |
100566 allocs/op |
100567 allocs/op |
1.00 |
BenchmarkDocument/tree_100 |
1015971 ns/op 943700 B/op 6101 allocs/op |
1071780 ns/op 943704 B/op 6101 allocs/op |
0.95 |
BenchmarkDocument/tree_100 - ns/op |
1015971 ns/op |
1071780 ns/op |
0.95 |
BenchmarkDocument/tree_100 - B/op |
943700 B/op |
943704 B/op |
1.00 |
BenchmarkDocument/tree_100 - allocs/op |
6101 allocs/op |
6101 allocs/op |
1 |
BenchmarkDocument/tree_1000 |
72523307 ns/op 86460317 B/op 60115 allocs/op |
79956298 ns/op 86460482 B/op 60115 allocs/op |
0.91 |
BenchmarkDocument/tree_1000 - ns/op |
72523307 ns/op |
79956298 ns/op |
0.91 |
BenchmarkDocument/tree_1000 - B/op |
86460317 B/op |
86460482 B/op |
1.00 |
BenchmarkDocument/tree_1000 - allocs/op |
60115 allocs/op |
60115 allocs/op |
1 |
BenchmarkDocument/tree_10000 |
9270046996 ns/op 8580670432 B/op 600220 allocs/op |
9897994875 ns/op 8580656176 B/op 600252 allocs/op |
0.94 |
BenchmarkDocument/tree_10000 - ns/op |
9270046996 ns/op |
9897994875 ns/op |
0.94 |
BenchmarkDocument/tree_10000 - B/op |
8580670432 B/op |
8580656176 B/op |
1.00 |
BenchmarkDocument/tree_10000 - allocs/op |
600220 allocs/op |
600252 allocs/op |
1.00 |
BenchmarkDocument/tree_delete_all_1000 |
77170656 ns/op 87509977 B/op 75266 allocs/op |
80032740 ns/op 87508876 B/op 75262 allocs/op |
0.96 |
BenchmarkDocument/tree_delete_all_1000 - ns/op |
77170656 ns/op |
80032740 ns/op |
0.96 |
BenchmarkDocument/tree_delete_all_1000 - B/op |
87509977 B/op |
87508876 B/op |
1.00 |
BenchmarkDocument/tree_delete_all_1000 - allocs/op |
75266 allocs/op |
75262 allocs/op |
1.00 |
BenchmarkDocument/tree_edit_gc_100 |
3823166 ns/op 4147770 B/op 15141 allocs/op |
3984044 ns/op 4146682 B/op 15141 allocs/op |
0.96 |
BenchmarkDocument/tree_edit_gc_100 - ns/op |
3823166 ns/op |
3984044 ns/op |
0.96 |
BenchmarkDocument/tree_edit_gc_100 - B/op |
4147770 B/op |
4146682 B/op |
1.00 |
BenchmarkDocument/tree_edit_gc_100 - allocs/op |
15141 allocs/op |
15141 allocs/op |
1 |
BenchmarkDocument/tree_edit_gc_1000 |
304508285 ns/op 383745414 B/op 154859 allocs/op |
321476574 ns/op 383745266 B/op 154868 allocs/op |
0.95 |
BenchmarkDocument/tree_edit_gc_1000 - ns/op |
304508285 ns/op |
321476574 ns/op |
0.95 |
BenchmarkDocument/tree_edit_gc_1000 - B/op |
383745414 B/op |
383745266 B/op |
1.00 |
BenchmarkDocument/tree_edit_gc_1000 - allocs/op |
154859 allocs/op |
154868 allocs/op |
1.00 |
BenchmarkDocument/tree_split_gc_100 |
2611145 ns/op 2412533 B/op 11125 allocs/op |
2711129 ns/op 2412501 B/op 11125 allocs/op |
0.96 |
BenchmarkDocument/tree_split_gc_100 - ns/op |
2611145 ns/op |
2711129 ns/op |
0.96 |
BenchmarkDocument/tree_split_gc_100 - B/op |
2412533 B/op |
2412501 B/op |
1.00 |
BenchmarkDocument/tree_split_gc_100 - allocs/op |
11125 allocs/op |
11125 allocs/op |
1 |
BenchmarkDocument/tree_split_gc_1000 |
189846441 ns/op 222252193 B/op 121999 allocs/op |
197945984 ns/op 222251521 B/op 122004 allocs/op |
0.96 |
BenchmarkDocument/tree_split_gc_1000 - ns/op |
189846441 ns/op |
197945984 ns/op |
0.96 |
BenchmarkDocument/tree_split_gc_1000 - B/op |
222252193 B/op |
222251521 B/op |
1.00 |
BenchmarkDocument/tree_split_gc_1000 - allocs/op |
121999 allocs/op |
122004 allocs/op |
1.00 |
BenchmarkRPC/client_to_server |
344269850 ns/op 17096754 B/op 170769 allocs/op |
349061437 ns/op 17655010 B/op 170839 allocs/op |
0.99 |
BenchmarkRPC/client_to_server - ns/op |
344269850 ns/op |
349061437 ns/op |
0.99 |
BenchmarkRPC/client_to_server - B/op |
17096754 B/op |
17655010 B/op |
0.97 |
BenchmarkRPC/client_to_server - allocs/op |
170769 allocs/op |
170839 allocs/op |
1.00 |
BenchmarkRPC/client_to_client_via_server |
632741126 ns/op 34335296 B/op 334607 allocs/op |
644490308 ns/op 34874144 B/op 331765 allocs/op |
0.98 |
BenchmarkRPC/client_to_client_via_server - ns/op |
632741126 ns/op |
644490308 ns/op |
0.98 |
BenchmarkRPC/client_to_client_via_server - B/op |
34335296 B/op |
34874144 B/op |
0.98 |
BenchmarkRPC/client_to_client_via_server - allocs/op |
334607 allocs/op |
331765 allocs/op |
1.01 |
BenchmarkRPC/attach_large_document |
1792153773 ns/op 3035339664 B/op 13254 allocs/op |
1812304287 ns/op 3021254280 B/op 12458 allocs/op |
0.99 |
BenchmarkRPC/attach_large_document - ns/op |
1792153773 ns/op |
1812304287 ns/op |
0.99 |
BenchmarkRPC/attach_large_document - B/op |
3035339664 B/op |
3021254280 B/op |
1.00 |
BenchmarkRPC/attach_large_document - allocs/op |
13254 allocs/op |
12458 allocs/op |
1.06 |
BenchmarkRPC/adminCli_to_server |
535007956 ns/op 36770536 B/op 289580 allocs/op |
537316904 ns/op 35952148 B/op 289571 allocs/op |
1.00 |
BenchmarkRPC/adminCli_to_server - ns/op |
535007956 ns/op |
537316904 ns/op |
1.00 |
BenchmarkRPC/adminCli_to_server - B/op |
36770536 B/op |
35952148 B/op |
1.02 |
BenchmarkRPC/adminCli_to_server - allocs/op |
289580 allocs/op |
289571 allocs/op |
1.00 |
BenchmarkLocker |
63.89 ns/op 16 B/op 1 allocs/op |
64.33 ns/op 16 B/op 1 allocs/op |
0.99 |
BenchmarkLocker - ns/op |
63.89 ns/op |
64.33 ns/op |
0.99 |
BenchmarkLocker - B/op |
16 B/op |
16 B/op |
1 |
BenchmarkLocker - allocs/op |
1 allocs/op |
1 allocs/op |
1 |
BenchmarkLockerParallel |
39.41 ns/op 0 B/op 0 allocs/op |
39.07 ns/op 0 B/op 0 allocs/op |
1.01 |
BenchmarkLockerParallel - ns/op |
39.41 ns/op |
39.07 ns/op |
1.01 |
BenchmarkLockerParallel - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkLockerParallel - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkLockerMoreKeys |
145.6 ns/op 15 B/op 0 allocs/op |
140.1 ns/op 15 B/op 0 allocs/op |
1.04 |
BenchmarkLockerMoreKeys - ns/op |
145.6 ns/op |
140.1 ns/op |
1.04 |
BenchmarkLockerMoreKeys - B/op |
15 B/op |
15 B/op |
1 |
BenchmarkLockerMoreKeys - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkChange/Push_10_Changes |
3746581 ns/op 117568 B/op 1215 allocs/op |
3815517 ns/op 117211 B/op 1215 allocs/op |
0.98 |
BenchmarkChange/Push_10_Changes - ns/op |
3746581 ns/op |
3815517 ns/op |
0.98 |
BenchmarkChange/Push_10_Changes - B/op |
117568 B/op |
117211 B/op |
1.00 |
BenchmarkChange/Push_10_Changes - allocs/op |
1215 allocs/op |
1215 allocs/op |
1 |
BenchmarkChange/Push_100_Changes |
15374384 ns/op 569233 B/op 6585 allocs/op |
15648137 ns/op 570426 B/op 6586 allocs/op |
0.98 |
BenchmarkChange/Push_100_Changes - ns/op |
15374384 ns/op |
15648137 ns/op |
0.98 |
BenchmarkChange/Push_100_Changes - B/op |
569233 B/op |
570426 B/op |
1.00 |
BenchmarkChange/Push_100_Changes - allocs/op |
6585 allocs/op |
6586 allocs/op |
1.00 |
BenchmarkChange/Push_1000_Changes |
125805094 ns/op 5208501 B/op 63077 allocs/op |
126200918 ns/op 5362366 B/op 63081 allocs/op |
1.00 |
BenchmarkChange/Push_1000_Changes - ns/op |
125805094 ns/op |
126200918 ns/op |
1.00 |
BenchmarkChange/Push_1000_Changes - B/op |
5208501 B/op |
5362366 B/op |
0.97 |
BenchmarkChange/Push_1000_Changes - allocs/op |
63077 allocs/op |
63081 allocs/op |
1.00 |
BenchmarkChange/Pull_10_Changes |
2989580 ns/op 103161 B/op 1018 allocs/op |
3039996 ns/op 102555 B/op 1018 allocs/op |
0.98 |
BenchmarkChange/Pull_10_Changes - ns/op |
2989580 ns/op |
3039996 ns/op |
0.98 |
BenchmarkChange/Pull_10_Changes - B/op |
103161 B/op |
102555 B/op |
1.01 |
BenchmarkChange/Pull_10_Changes - allocs/op |
1018 allocs/op |
1018 allocs/op |
1 |
BenchmarkChange/Pull_100_Changes |
4566810 ns/op 268225 B/op 3489 allocs/op |
4590477 ns/op 266845 B/op 3488 allocs/op |
0.99 |
BenchmarkChange/Pull_100_Changes - ns/op |
4566810 ns/op |
4590477 ns/op |
0.99 |
BenchmarkChange/Pull_100_Changes - B/op |
268225 B/op |
266845 B/op |
1.01 |
BenchmarkChange/Pull_100_Changes - allocs/op |
3489 allocs/op |
3488 allocs/op |
1.00 |
BenchmarkChange/Pull_1000_Changes |
8620347 ns/op 1492772 B/op 29870 allocs/op |
8911691 ns/op 1490386 B/op 29867 allocs/op |
0.97 |
BenchmarkChange/Pull_1000_Changes - ns/op |
8620347 ns/op |
8911691 ns/op |
0.97 |
BenchmarkChange/Pull_1000_Changes - B/op |
1492772 B/op |
1490386 B/op |
1.00 |
BenchmarkChange/Pull_1000_Changes - allocs/op |
29870 allocs/op |
29867 allocs/op |
1.00 |
BenchmarkSnapshot/Push_3KB_snapshot |
17849003 ns/op 709406 B/op 6586 allocs/op |
18044547 ns/op 708528 B/op 6586 allocs/op |
0.99 |
BenchmarkSnapshot/Push_3KB_snapshot - ns/op |
17849003 ns/op |
18044547 ns/op |
0.99 |
BenchmarkSnapshot/Push_3KB_snapshot - B/op |
709406 B/op |
708528 B/op |
1.00 |
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op |
6586 allocs/op |
6586 allocs/op |
1 |
BenchmarkSnapshot/Push_30KB_snapshot |
129742374 ns/op 5542497 B/op 63078 allocs/op |
129117558 ns/op 5586802 B/op 63079 allocs/op |
1.00 |
BenchmarkSnapshot/Push_30KB_snapshot - ns/op |
129742374 ns/op |
129117558 ns/op |
1.00 |
BenchmarkSnapshot/Push_30KB_snapshot - B/op |
5542497 B/op |
5586802 B/op |
0.99 |
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op |
63078 allocs/op |
63079 allocs/op |
1.00 |
BenchmarkSnapshot/Pull_3KB_snapshot |
6776254 ns/op 924015 B/op 15524 allocs/op |
6690573 ns/op 921973 B/op 15522 allocs/op |
1.01 |
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op |
6776254 ns/op |
6690573 ns/op |
1.01 |
BenchmarkSnapshot/Pull_3KB_snapshot - B/op |
924015 B/op |
921973 B/op |
1.00 |
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op |
15524 allocs/op |
15522 allocs/op |
1.00 |
BenchmarkSnapshot/Pull_30KB_snapshot |
15384007 ns/op 7158354 B/op 150119 allocs/op |
15789110 ns/op 7145336 B/op 150119 allocs/op |
0.97 |
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op |
15384007 ns/op |
15789110 ns/op |
0.97 |
BenchmarkSnapshot/Pull_30KB_snapshot - B/op |
7158354 B/op |
7145336 B/op |
1.00 |
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op |
150119 allocs/op |
150119 allocs/op |
1 |
BenchmarkSplayTree/stress_test_100000 |
0.1952 ns/op 0 B/op 0 allocs/op |
0.1869 ns/op 0 B/op 0 allocs/op |
1.04 |
BenchmarkSplayTree/stress_test_100000 - ns/op |
0.1952 ns/op |
0.1869 ns/op |
1.04 |
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.378 ns/op 0 B/op 0 allocs/op |
0.3992 ns/op 0 B/op 0 allocs/op |
0.95 |
BenchmarkSplayTree/stress_test_200000 - ns/op |
0.378 ns/op |
0.3992 ns/op |
0.95 |
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.5881 ns/op 0 B/op 0 allocs/op |
0.5808 ns/op 0 B/op 0 allocs/op |
1.01 |
BenchmarkSplayTree/stress_test_300000 - ns/op |
0.5881 ns/op |
0.5808 ns/op |
1.01 |
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.01257 ns/op 0 B/op 0 allocs/op |
0.01302 ns/op 0 B/op 0 allocs/op |
0.97 |
BenchmarkSplayTree/random_access_100000 - ns/op |
0.01257 ns/op |
0.01302 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.02372 ns/op 0 B/op 0 allocs/op |
0.02622 ns/op 0 B/op 0 allocs/op |
0.90 |
BenchmarkSplayTree/random_access_200000 - ns/op |
0.02372 ns/op |
0.02622 ns/op |
0.90 |
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.03603 ns/op 0 B/op 0 allocs/op |
0.03808 ns/op 0 B/op 0 allocs/op |
0.95 |
BenchmarkSplayTree/random_access_300000 - ns/op |
0.03603 ns/op |
0.03808 ns/op |
0.95 |
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.001953 ns/op 0 B/op 0 allocs/op |
0.002446 ns/op 0 B/op 0 allocs/op |
0.80 |
BenchmarkSplayTree/editing_trace_bench - ns/op |
0.001953 ns/op |
0.002446 ns/op |
0.80 |
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 |
6858 ns/op 1286 B/op 38 allocs/op |
6945 ns/op 1286 B/op 38 allocs/op |
0.99 |
BenchmarkSync/memory_sync_10_test - ns/op |
6858 ns/op |
6945 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 |
50858 ns/op 8646 B/op 273 allocs/op |
51079 ns/op 8641 B/op 273 allocs/op |
1.00 |
BenchmarkSync/memory_sync_100_test - ns/op |
50858 ns/op |
51079 ns/op |
1.00 |
BenchmarkSync/memory_sync_100_test - B/op |
8646 B/op |
8641 B/op |
1.00 |
BenchmarkSync/memory_sync_100_test - allocs/op |
273 allocs/op |
273 allocs/op |
1 |
BenchmarkSync/memory_sync_1000_test |
588257 ns/op 74054 B/op 2103 allocs/op |
581015 ns/op 74870 B/op 2152 allocs/op |
1.01 |
BenchmarkSync/memory_sync_1000_test - ns/op |
588257 ns/op |
581015 ns/op |
1.01 |
BenchmarkSync/memory_sync_1000_test - B/op |
74054 B/op |
74870 B/op |
0.99 |
BenchmarkSync/memory_sync_1000_test - allocs/op |
2103 allocs/op |
2152 allocs/op |
0.98 |
BenchmarkSync/memory_sync_10000_test |
7347513 ns/op 744872 B/op 20288 allocs/op |
7733341 ns/op 737529 B/op 20266 allocs/op |
0.95 |
BenchmarkSync/memory_sync_10000_test - ns/op |
7347513 ns/op |
7733341 ns/op |
0.95 |
BenchmarkSync/memory_sync_10000_test - B/op |
744872 B/op |
737529 B/op |
1.01 |
BenchmarkSync/memory_sync_10000_test - allocs/op |
20288 allocs/op |
20266 allocs/op |
1.00 |
BenchmarkTextEditing |
5059291771 ns/op 3903617872 B/op 19608506 allocs/op |
4945228774 ns/op 3903643616 B/op 19608559 allocs/op |
1.02 |
BenchmarkTextEditing - ns/op |
5059291771 ns/op |
4945228774 ns/op |
1.02 |
BenchmarkTextEditing - B/op |
3903617872 B/op |
3903643616 B/op |
1.00 |
BenchmarkTextEditing - allocs/op |
19608506 allocs/op |
19608559 allocs/op |
1.00 |
BenchmarkTree/10000_vertices_to_protobuf |
3568073 ns/op 6262973 B/op 70025 allocs/op |
3584593 ns/op 6263033 B/op 70025 allocs/op |
1.00 |
BenchmarkTree/10000_vertices_to_protobuf - ns/op |
3568073 ns/op |
3584593 ns/op |
1.00 |
BenchmarkTree/10000_vertices_to_protobuf - B/op |
6262973 B/op |
6263033 B/op |
1.00 |
BenchmarkTree/10000_vertices_to_protobuf - allocs/op |
70025 allocs/op |
70025 allocs/op |
1 |
BenchmarkTree/10000_vertices_from_protobuf |
158771096 ns/op 442172620 B/op 290039 allocs/op |
161101715 ns/op 442171446 B/op 290039 allocs/op |
0.99 |
BenchmarkTree/10000_vertices_from_protobuf - ns/op |
158771096 ns/op |
161101715 ns/op |
0.99 |
BenchmarkTree/10000_vertices_from_protobuf - B/op |
442172620 B/op |
442171446 B/op |
1.00 |
BenchmarkTree/10000_vertices_from_protobuf - allocs/op |
290039 allocs/op |
290039 allocs/op |
1 |
BenchmarkTree/20000_vertices_to_protobuf |
7804219 ns/op 12716926 B/op 140028 allocs/op |
8073991 ns/op 12721130 B/op 140028 allocs/op |
0.97 |
BenchmarkTree/20000_vertices_to_protobuf - ns/op |
7804219 ns/op |
8073991 ns/op |
0.97 |
BenchmarkTree/20000_vertices_to_protobuf - B/op |
12716926 B/op |
12721130 B/op |
1.00 |
BenchmarkTree/20000_vertices_to_protobuf - allocs/op |
140028 allocs/op |
140028 allocs/op |
1 |
BenchmarkTree/20000_vertices_from_protobuf |
699299415 ns/op 1697267960 B/op 580043 allocs/op |
683105482 ns/op 1697267704 B/op 580042 allocs/op |
1.02 |
BenchmarkTree/20000_vertices_from_protobuf - ns/op |
699299415 ns/op |
683105482 ns/op |
1.02 |
BenchmarkTree/20000_vertices_from_protobuf - B/op |
1697267960 B/op |
1697267704 B/op |
1.00 |
BenchmarkTree/20000_vertices_from_protobuf - allocs/op |
580043 allocs/op |
580042 allocs/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf |
12323406 ns/op 19324633 B/op 210030 allocs/op |
12502493 ns/op 19318234 B/op 210030 allocs/op |
0.99 |
BenchmarkTree/30000_vertices_to_protobuf - ns/op |
12323406 ns/op |
12502493 ns/op |
0.99 |
BenchmarkTree/30000_vertices_to_protobuf - B/op |
19324633 B/op |
19318234 B/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf - allocs/op |
210030 allocs/op |
210030 allocs/op |
1 |
BenchmarkTree/30000_vertices_from_protobuf |
1634736850 ns/op 3752052888 B/op 870136 allocs/op |
1629191080 ns/op 3752044008 B/op 870048 allocs/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf - ns/op |
1634736850 ns/op |
1629191080 ns/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf - B/op |
3752052888 B/op |
3752044008 B/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf - allocs/op |
870136 allocs/op |
870048 allocs/op |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
af4b0d8 to
f7d2e19
Compare
f7d2e19 to
e1782a3
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (17)
api/yorkie/v1/system.proto (1)
30-36: LGTM: Request message structure is appropriate.The
SystemServiceDetachDocumentRequestmessage is well-defined with clear and descriptive field names.Regarding the TODO comment:
Consider creating a separate issue to track the potential enhancement of replacing the string fields with more complex types (
types.Project,types.Client,types.DocumentSummary). This will ensure the improvement isn't forgotten and can be prioritized appropriately in the future.server/clients/clients.go (1)
Action Required: Missing
projectParameter inActivateFunction CallsThe
Activatefunction has been updated to include theproject *types.Projectparameter. However, the following calls are missing this new parameter and need to be updated accordingly:
test/integration/server_test.go: assert.NoError(t, cli.Activate(ctx))test/integration/retention_test.go: assert.NoError(t, cli.Activate(context.Background()))test/integration/client_test.go: assert.NoError(t, cli.Activate(ctx))test/integration/auth_webhook_test.go: assert.NoError(t, cli.Activate(ctx))test/integration/admin_test.go: assert.NoError(t, cli.Activate(ctx))test/integration/gc_test.go: assert.NoError(t, c1.Activate(ctx))test/integration/document_test.go: assert.NoError(t, cli.Activate(ctx))test/bench/grpc_bench_test.go: err = c.Activate(context.Background())test/helper/helper.go: assert.NoError(t, cli.Activate(ctx))test/complex/main_test.go: assert.NoError(t, c.Activate(context.Background()))Please update all the above instances to include the new
projectparameter to ensure consistent and error-free function calls.🔗 Analysis chain
Line range hint
38-46: LGTM: Added project parameter to Activate function.The addition of the
project *types.Projectparameter to theActivatefunction is appropriate. This change allows for project-specific client activation.Please ensure that all calls to the
Activatefunction have been updated to include the newprojectparameter. Run the following script to verify:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `Activate` include the new project parameter. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'Activate\s*\('Length of output: 8592
Script:
#!/bin/bash # Description: Verify all function calls to `Activate` include the new project parameter. # Corrected Test: Search for function calls to `Activate` with the new `project` parameter. rg --type go 'Activate\s*\([^)]*project\s*\*types\.Project[^)]*\)'Length of output: 68
Script:
#!/bin/bash # Description: Verify all function calls to `Activate` include the new project parameter. # Updated Test: Search for all `Activate` function calls and check if they include the `project` parameter. # This regex looks for `Activate(` followed by any number of arguments and ensures `project` is one of them. rg --type go 'Activate\s*\(([^)]*,\s*)?project\s*\*types\.Project([^)]*)?\)'Length of output: 78
Script:
#!/bin/bash # Description: List all `Activate` function calls to verify inclusion of the new `project` parameter. # Search for all `Activate` function calls in Go files and display their arguments. rg --type go 'Activate\s*\(([^)]*)\)' --context 2Length of output: 10398
server/clients/housekeeping.go (3)
71-72: LGTM! Consider improving error handling.The changes correctly adapt the loop to use the new
CandidatePairstruct. However, as previously suggested, consider handling deactivation errors individually to improve robustness. Currently, if deactivating a single client fails, the function returns immediately and skips the rest of the candidates.You could modify the loop to collect errors:
var deactivateErrors []error for _, pair := range candidates { if _, err := Deactivate(ctx, be, pair.Project.ToProject(), pair.Client.RefKey()); err != nil { deactivateErrors = append(deactivateErrors, err) } } if len(deactivateErrors) > 0 { // Handle or log errors appropriately return database.DefaultProjectID, fmt.Errorf("errors during deactivation: %v", deactivateErrors) }
104-105: LGTM! Update function documentation.The change in return type from
[]*database.ClientInfoto[]CandidatePairis consistent with the introduction of theCandidatePairstruct. However, as previously suggested, please update the function's documentation to reflect this change in the return type.Update the function comment to reflect the new return type:
// FindDeactivateCandidates finds candidates to deactivate from the database. // It returns the ID of the last processed project and a slice of CandidatePair structs.
110-122: LGTM! Consider optimizing slice allocation.The changes correctly implement the use of
CandidatePairstructs and improve the clarity of the code. However, as previously suggested, consider optimizing the candidate collection by preallocating the slice capacity, especially if dealing with a large number of candidates.You could preallocate the
candidatesslice with an estimated capacity:estimatedTotalCandidates := len(projectInfos) * candidatesLimitPerProject candidates := make([]CandidatePair, 0, estimatedTotalCandidates) for _, projectInfo := range projectInfos { infos, err := be.DB.FindDeactivateCandidatesPerProject(ctx, projectInfo, candidatesLimitPerProject) if err != nil { return database.DefaultProjectID, nil, err } for _, info := range infos { candidates = append(candidates, CandidatePair{ Project: projectInfo, Client: info, }) } }server/config.sample.yml (1)
90-92: Add a descriptive comment for the newGatewayAddrparameter.While the addition of the
GatewayAddrparameter is correctly placed and follows the existing naming conventions, it lacks a descriptive comment. To improve clarity and ease of use, please add a comment explaining:
- The purpose of this parameter
- The expected format of the address (e.g., "host:port" or full URL)
- Whether this is a required or optional parameter
- Any default behavior if left empty
Example:
# GatewayAddr is the address of the gateway server. # Format: "host:port" or full URL. Optional. If not provided, the server will not use a gateway. GatewayAddr: ""This will help users understand how to properly configure this new option.
api/docs/yorkie/v1/system.openapi.yaml (4)
7-11: LGTM: Server definitions are clear and appropriate.The server definitions for both production and local environments are well-defined. The use of HTTPS for the production server is a good security practice.
Consider adding a staging or testing server URL if applicable to your development workflow.
12-24: LGTM: Path definition for detaching documents is well-structured.The path definition follows RESTful conventions and includes appropriate references for request and response bodies. The inclusion of a default error response is a good practice.
Consider adding a brief description for the endpoint in the
descriptionfield (line 15) to provide more context about its functionality.
25-110: LGTM: Components are well-structured, but TODO needs addressing.The components section is well-organized, with clear definitions for request bodies, responses, and schemas. The error schema is particularly comprehensive.
Please address the TODO comment in the
yorkie.v1.SystemServiceDetachDocumentRequestschema (lines 105-106). Consider replacing the fields withtypes.Project,types.Client, andtypes.DocumentSummaryas suggested.
116-122: LGTM: Security scheme is well-defined.The API key authentication scheme is appropriately defined using the Authorization header, which is a standard practice.
Consider adding a brief description to the security scheme to provide more context about the expected API key format or any specific requirements.
server/backend/config.go (1)
Line range hint
32-32: Fix typo in YAML tag forSnapshotWithPurgingChangesThere's a typo in the YAML tag for the
SnapshotWithPurgingChangesfield. It currently reads "SnapshotWithPurgingChages", missing an 'n' in "Changes".Please apply the following fix:
- SnapshotWithPurgingChanges bool `yaml:"SnapshotWithPurgingChages"` + SnapshotWithPurgingChanges bool `yaml:"SnapshotWithPurgingChanges"`This correction ensures proper YAML serialization and deserialization of the configuration.
server/config.go (1)
71-71: Consider consistent formatting across constants.The spacing before the
=sign forDefaultHostnamehas been modified. While this doesn't affect functionality, it's important to maintain consistent formatting throughout the codebase. Please ensure this change aligns with the project's style guide or formatting standards.server/documents/documents.go (1)
138-138: LGTM. Consider addressing the N+1 problem.The change from
packs.BuildDocumentForServerSeqtopacks.BuildInternalDocForServerSeqis consistent with other functions.There's a TODO comment about resolving an N+1 problem. Consider creating an issue to track this performance improvement:
// TODO(hackerwins, kokodak): Resolve the N+1 problem.Would you like me to create a GitHub issue to track this performance improvement?
test/helper/helper.go (1)
259-259: LGTM! Consider adding a comment for clarity.The addition of the
GatewayAddrfield to theBackendconfiguration is well-implemented and consistent with the existing code style. It provides a unique gateway address for each test instance, which is beneficial for parallel testing scenarios.Consider adding a brief comment explaining the purpose of the
GatewayAddrfield and its relationship to theRPCPort. This would enhance code readability and make it easier for other developers to understand the configuration's intent. For example:+ // GatewayAddr specifies the address for the gateway in the test environment. + // It uses the same port as the RPC server to simplify the test setup. GatewayAddr: fmt.Sprintf("localhost:%d", RPCPort+portOffset),server/rpc/yorkie_server.go (1)
97-100: LGTM! Consider enhancing error handling.The addition of the
projectparameter to theclients.Deactivatefunction call is a good improvement. It provides more context for the deactivation process, which aligns well with the PR objective of detaching documents when a client is deactivated.Consider adding a check to ensure that
projectis not nil before using it:project := projects.From(ctx) +if project == nil { + return nil, fmt.Errorf("project not found in context") +} _, err = clients.Deactivate(ctx, s.backend, project, types.ClientRefKey{ ProjectID: project.ID, ClientID: types.IDFromActorID(actorID), })This additional check would prevent potential nil pointer dereferences and provide a more informative error message if the project is missing from the context.
server/packs/packs.go (2)
Line range hint
233-278: Prioritize handling the TODO for large change setsThe
BuildInternalDocForServerSeqfunction contains a TODO comment regarding the potential performance impact when applying a large number of changes if the snapshot is missing. Processing all changes at once can lead to high memory usage and prolonged processing time.Consider implementing a mechanism to batch the application of changes, processing them in smaller chunks (e.g., 100 changes at a time). This approach can improve performance and reduce resource consumption.
Would you like assistance in drafting an implementation plan or creating a GitHub issue to track this enhancement?
Line range hint
233-278: Review logging of document content in debug modeIn the
BuildInternalDocForServerSeqfunction, sensitive document content is being logged when debug logging is enabled:logging.From(ctx).Debugf( "after apply %d changes: elements: %d removeds: %d, %s", len(changes), doc.Root().ElementMapLen(), doc.Root().GarbageElementLen(), doc.RootObject().Marshal(), )While detailed logs are valuable for debugging, logging the entire document content may expose sensitive information.
Consider modifying the logging statement to exclude or mask sensitive data:
logging.From(ctx).Debugf( "after apply %d changes: elements: %d removeds: %d", len(changes), doc.Root().ElementMapLen(), doc.Root().GarbageElementLen(), - doc.RootObject().Marshal(), )
🛑 Comments failed to post (8)
api/yorkie/v1/system.proto (2)
18-18:
⚠️ Potential issueConsider adjusting the package directory structure.
The static analysis tool indicates that files with package "yorkie.v1" should be within a directory "yorkie/v1" relative to the root, but they are currently in the "api/yorkie/v1" directory. This mismatch might cause issues with some tools or conventions.
Consider one of the following options:
- Move this file to the "yorkie/v1" directory.
- Adjust the package name to match the current directory structure.
- If the current structure is intentional, update the configuration of the static analysis tool to ignore this warning.
Please clarify the intended package structure for the project.
🧰 Tools
🪛 buf
18-18: Files with package "yorkie.v1" must be within a directory "yorkie/v1" relative to root but were in directory "api/yorkie/v1".
(PACKAGE_DIRECTORY_MATCH)
38-38: 🛠️ Refactor suggestion
Consider adding a status field to the response message.
The
SystemServiceDetachDocumentResponsemessage is currently empty. While this might be intentional if no data needs to be returned, it's often useful to include at least a status field in the response.Consider adding a status field to confirm the success of the operation:
-message SystemServiceDetachDocumentResponse {} +message SystemServiceDetachDocumentResponse { + bool success = 1; +}This addition would provide explicit confirmation that the document was successfully detached.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.message SystemServiceDetachDocumentResponse { bool success = 1; }server/rpc/system_server.go (4)
115-131: 🛠️ Refactor suggestion
Reminder: Maintain consistent error handling.
Continue the practice of wrapping errors with additional context in this final section of the method.
For example:
if _, err := packs.PushPull( ctx, s.backend, project, clientInfo, docInfo, doc.CreateChangePack(), packs.PushPullOptions{ Mode: types.SyncModePushPull, Status: document.StatusDetached, }, ); err != nil { - return nil, err + return nil, fmt.Errorf("failed to push changes: %w", err) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if _, err := packs.PushPull( ctx, s.backend, project, clientInfo, docInfo, doc.CreateChangePack(), packs.PushPullOptions{ Mode: types.SyncModePushPull, Status: document.StatusDetached, }, ); err != nil { return nil, fmt.Errorf("failed to push changes: %w", err) } return connect.NewResponse(&api.SystemServiceDetachDocumentResponse{}), nil }
85-113:
⚠️ Potential issueAdd nil check before clearing presence.
To prevent potential nil pointer dereferences, add a nil check before clearing the presence.
Apply this change:
if err := doc.Update(func(root *json.Object, p *presence.Presence) error { + if p != nil { p.Clear() + } return nil }); err != nil { return nil, err }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.clientInfo, err := clients.FindActiveClientInfo(ctx, s.backend.DB, types.ClientRefKey{ ProjectID: project.ID, ClientID: types.IDFromActorID(actorID), }) if err != nil { return nil, err } docRefKey := types.DocRefKey{ ProjectID: project.ID, DocID: docID, } docInfo, err := documents.FindDocInfoByRefKey(ctx, s.backend, docRefKey) if err != nil { return nil, err } doc, err := packs.BuildDocForCheckpoint(ctx, s.backend, docInfo, clientInfo.Checkpoint(docID), actorID) if err != nil { return nil, err } if err := doc.Update(func(root *json.Object, p *presence.Presence) error { if p != nil { p.Clear() } return nil }); err != nil { return nil, err }
85-113: 🛠️ Refactor suggestion
Reminder: Ensure consistent error handling.
As mentioned earlier, consider wrapping errors with additional context throughout this section for better debugging.
For example:
clientInfo, err := clients.FindActiveClientInfo(ctx, s.backend.DB, types.ClientRefKey{ ProjectID: project.ID, ClientID: types.IDFromActorID(actorID), }) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to find active client info: %w", err) }Apply similar changes to other error returns in this section.
Committable suggestion was skipped due to low confidence.
50-83: 🛠️ Refactor suggestion
Enhance error handling for better debugging.
While the error handling is present, it could be improved to provide more context for debugging. Consider wrapping errors with additional information.
Apply this pattern to error returns:
actorID, err := time.ActorIDFromHex(req.Msg.ClientId) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse actor ID: %w", err) } docID, err := converter.FromDocumentID(req.Msg.DocumentId) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse document ID: %w", err) }Apply similar changes to other error returns within the function to enhance error reporting.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// DetachDocument detaches the given document from the given client. func (s *systemServer) DetachDocument( ctx context.Context, req *connect.Request[api.SystemServiceDetachDocumentRequest], ) (*connect.Response[api.SystemServiceDetachDocumentResponse], error) { actorID, err := time.ActorIDFromHex(req.Msg.ClientId) if err != nil { return nil, fmt.Errorf("failed to parse actor ID: %w", err) } docID, err := converter.FromDocumentID(req.Msg.DocumentId) if err != nil { return nil, fmt.Errorf("failed to parse document ID: %w", err) } info, err := s.backend.DB.FindProjectInfoByID(ctx, types.ID(req.Msg.ProjectId)) if err != nil { return nil, fmt.Errorf("failed to find project info: %w", err) } project := info.ToProject() locker, err := s.backend.Coordinator.NewLocker(ctx, packs.PushPullKey(project.ID, key.Key(req.Msg.DocumentKey))) if err != nil { return nil, fmt.Errorf("failed to create locker: %w", err) } if err := locker.Lock(ctx); err != nil { return nil, fmt.Errorf("failed to acquire lock: %w", err) } defer func() { if err := locker.Unlock(ctx); err != nil { logging.DefaultLogger().Error(fmt.Errorf("failed to release lock: %w", err)) } }()server/backend/config.go (1)
85-86: 💡 Codebase verification
Add Validation for
GatewayAddrThe
GatewayAddrfield is actively utilized in several parts of the codebase, includingcmd/yorkie/server.goandserver/clients/clients.go. However, there is currently no validation in theValidate()method to ensure thatGatewayAddris correctly formatted or reachable. To maintain robustness, please add appropriate validation for this field.🔗 Analysis chain
LGTM. Consider adding validation and clarifying the purpose.
The addition of the
GatewayAddrfield is appropriate. However, consider the following suggestions:
- Add validation for this field in the
Validate()method, especially if it's required or has a specific format.- Provide more context about the purpose and impact of this gateway server in the comment.
To ensure this new field is properly utilized, let's check for its usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of GatewayAddr in the codebase rg --type go 'GatewayAddr'Length of output: 539
server/packs/packs.go (1)
215-231:
⚠️ Potential issueCheck for nil
actorIDbefore setting the actorIn the
BuildDocForCheckpointfunction, consider verifying thatactorIDis notnilbefore callinginternalDoc.SetActor(actorID). This precaution prevents potential nil pointer dereferences, enhancing the robustness of the code.Apply this diff to include a nil check:
func BuildDocForCheckpoint( ctx context.Context, be *backend.Backend, docInfo *database.DocInfo, cp change.Checkpoint, actorID *time.ActorID, ) (*document.Document, error) { internalDoc, err := BuildInternalDocForServerSeq(ctx, be, docInfo, cp.ServerSeq) if err != nil { return nil, err } + if actorID == nil { + return nil, fmt.Errorf("actorID is nil") + } internalDoc.SetActor(actorID) internalDoc.SyncCheckpoint(cp.ServerSeq, cp.ClientSeq) return internalDoc.ToDocument(), nil }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// BuildDocForCheckpoint returns a new document for the given checkpoint. func BuildDocForCheckpoint( ctx context.Context, be *backend.Backend, docInfo *database.DocInfo, cp change.Checkpoint, actorID *time.ActorID, ) (*document.Document, error) { internalDoc, err := BuildInternalDocForServerSeq(ctx, be, docInfo, cp.ServerSeq) if err != nil { return nil, err } if actorID == nil { return nil, fmt.Errorf("actorID is nil") } internalDoc.SetActor(actorID) internalDoc.SyncCheckpoint(cp.ServerSeq, cp.ClientSeq) return internalDoc.ToDocument(), nil }
917bd2e to
56c2073
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
build/charts/yorkie-cluster/values.yaml (1)
6-7: LGTM! Consider adding a comment for clarity.The addition of the
gatewayparameter under theyorkiesection is appropriate and aligns with the PR objectives. It likely specifies the gateway service to be used for the Yorkie cluster.Consider adding a brief comment above this line to explain the purpose of the
gatewayparameter, for example:# Specify the gateway service for the Yorkie cluster gateway: yorkie-gatewayThis would enhance the configuration file's readability and maintainability.
api/yorkie/v1/system.proto (3)
18-18: Consider adjusting the package directory structure.The package "yorkie.v1" is currently located in the directory "api/yorkie/v1". According to the static analysis tool, it should be in a directory "yorkie/v1" relative to the root. This mismatch might cause issues with some tools or conventions.
Consider one of the following options:
- Move the file to the correct directory structure.
- If the current structure is intentional, document the reason for deviating from the standard convention.
🧰 Tools
🪛 buf
18-18: Files with package "yorkie.v1" must be within a directory "yorkie/v1" relative to root but were in directory "api/yorkie/v1".
(PACKAGE_DIRECTORY_MATCH)
30-36: Consider creating an issue to track the TODO comment.The TODO comment suggests replacing the current string fields with more complex types like
types.Project,types.Client, andtypes.DocumentSummary. This could lead to a more robust and type-safe implementation in the future.Would you like me to create a GitHub issue to track this potential enhancement? This would help ensure it's not forgotten and can be addressed in a future update.
38-38: Consider adding a status field to the response message.The
SystemServiceDetachDocumentResponsemessage is currently empty. While this might be intentional if no response data is needed, it's generally useful to include at least a status field or some indication of the operation's success.Consider adding a field such as:
message SystemServiceDetachDocumentResponse { bool success = 1; // Optionally, you could also include an error message: // string error_message = 2; }This would provide confirmation that the document was successfully detached or indicate if there was an issue.
build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1)
Line range hint
52-53: Minor indentation improvementThe indentation of the
containerPortentries has been corrected, improving the YAML formatting and readability of the file.server/clients/housekeeping.go (1)
Line range hint
1-133: Overall, good improvements with room for further enhancements.The introduction of
CandidatePairand the corresponding changes improve the code structure and clarity. However, there are still opportunities for enhancement:
- Improve error handling in
DeactivateInactivesto handle individual deactivation errors.- Update the documentation for
FindDeactivateCandidatesto reflect the new return type.- Consider optimizing slice allocation in
FindDeactivateCandidates.Addressing these points would further improve the robustness and efficiency of the code.
server/packs/packs.go (2)
215-231: LGTM with a minor suggestion for error handling.The
BuildDocForCheckpointfunction is well-structured and follows good practices. It effectively uses the newBuildInternalDocForServerSeqfunction, which improves modularity. The function signature is clear, and it correctly handles the context and error checking.Consider wrapping the error returned from
BuildInternalDocForServerSeqwith additional context:internalDoc, err := BuildInternalDocForServerSeq(ctx, be, docInfo, cp.ServerSeq) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to build internal doc: %w", err) }This will provide more context if an error occurs, making debugging easier.
Line range hint
215-290: Summary: Improved document building process with potential for further optimizationThe introduced changes enhance the document building process by adding two new functions:
BuildDocForCheckpointandBuildInternalDocForServerSeq. These functions provide a more flexible and modular approach to document construction based on checkpoints and server sequences.While these changes align with the overall goal of improving document handling, they don't directly implement the PR objective of detaching documents when a client is deactivated. This functionality is likely handled elsewhere in the codebase.
Consider the following recommendations for future improvements:
- Address the TODO comment in
BuildInternalDocForServerSeqto optimize handling of large change sets.- Implement comprehensive unit tests for these new functions to ensure their correctness and performance under various scenarios.
- Document the relationship between these new functions and the document detachment process mentioned in the PR objectives.
These changes lay a good foundation for improved document synchronization, but careful attention to performance optimization and thorough testing will be crucial as the system scales.
server/rpc/yorkie_server.go (1)
97-100: LGTM! Consider a minor improvement for consistency.The addition of the
projectparameter to theclients.Deactivatefunction call is a good improvement. It provides more context for the deactivation process, which aligns with the PR objective of detaching documents when a client is deactivated.For consistency with other methods in this file, consider moving the
project := projects.From(ctx)line just before theclients.Deactivatecall. This would make the code more readable and maintain a consistent pattern throughout the file.Here's a suggested minor refactor:
func (s *yorkieServer) DeactivateClient( ctx context.Context, req *connect.Request[api.DeactivateClientRequest], ) (*connect.Response[api.DeactivateClientResponse], error) { actorID, err := time.ActorIDFromHex(req.Msg.ClientId) if err != nil { return nil, err } if err := auth.VerifyAccess(ctx, s.backend, &types.AccessInfo{ Method: types.DeactivateClient, }); err != nil { return nil, err } - project := projects.From(ctx) _, err = clients.Deactivate(ctx, s.backend, project, types.ClientRefKey{ + project := projects.From(ctx) + _, err = clients.Deactivate(ctx, s.backend, project, types.ClientRefKey{ ProjectID: project.ID, ClientID: types.IDFromActorID(actorID), }) if err != nil { return nil, err } return connect.NewResponse(&api.DeactivateClientResponse{}), nil }server/clients/clients.go (1)
Missing
projectParameter inActivateFunction CallsSeveral calls to the
Activatefunction do not include the requiredprojectparameter:
test/integration/retention_test.gotest/integration/main_test.gotest/integration/gc_test.gotest/integration/client_test.gotest/integration/document_test.gotest/integration/admin_test.gotest/integration/auth_webhook_test.gotest/helper/helper.gotest/bench/grpc_bench_test.gotest/integration/server_test.gotest/complex/main_test.goclient/client_test.goAction Required: Update all instances of the
Activatefunction to include the newprojectparameter to prevent potential runtime errors.🔗 Analysis chain
Line range hint
38-43: Ensure all callers ofActivateare updated with the newprojectparameterThe
Activatefunction signature now includesproject *types.Projectas a parameter. Please verify that all calls toActivatein the codebase have been updated accordingly to prevent any runtime errors due to mismatched parameters.To confirm that all callers have been updated, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to the `Activate` function and check if they include the new `project` parameter. # Search for calls to Activate in Go files, excluding the function definition itself. rg --type go --glob '!.*/clients.go' 'Activate\(' -A 1Length of output: 4709
🛑 Comments failed to post (5)
server/clients/clients.go (3)
60-65:
⚠️ Potential issueEvaluate the use of
WithInsecure(true)for system client connectionsThe
systemClientis initialized withsystem.WithInsecure(true), which disables transport security for gRPC connections. In production environments, using insecure connections can expose the system to security risks such as man-in-the-middle attacks. Consider configuring the connection to use secure credentials or making the insecurity optional based on the environment (e.g., development vs. production).To enhance security, you might update the initialization as follows:
-systemClient, err := system.Dial(be.Config.GatewayAddr, system.WithInsecure(true)) +systemClient, err := system.Dial(be.Config.GatewayAddr)Ensure that the
system.Dialfunction and your environment are configured to handle secure connections appropriately.Committable suggestion was skipped due to low confidence.
98-98:
⚠️ Potential issueCorrect grammatical error in the function comment
The comment for
FindActiveClientInfoshould be updated for grammatical accuracy and to follow Go's convention for documentation comments.Apply this diff to fix the comment:
-// FindActiveClientInfo find the active Client info by the given ref key. +// FindActiveClientInfo finds the active client info by the given reference key.This ensures the comment is clear and follows proper grammar.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// FindActiveClientInfo finds the active client info by the given reference key.
48-54: 💡 Codebase verification
Outdated Calls to
DeactivateFunction FoundSeveral calls to
Deactivateare missing the newbeandprojectparameters. Please update them to match the new function signature to prevent runtime errors.Affected files:
test/integration/retention_test.gotest/integration/main_test.gotest/integration/client_test.gotest/integration/auth_webhook_test.gotest/integration/document_test.gotest/integration/gc_test.gotest/complex/main_test.gotest/bench/grpc_bench_test.goclient/client.go🔗 Analysis chain
Ensure all callers of
Deactivateare updated with the newbeandprojectparametersThe
Deactivatefunction signature now includesbe *backend.Backendandproject *types.Projectas parameters. Please verify that all calls toDeactivateacross the codebase have been updated to pass these new parameters to prevent any mismatches or runtime errors.To check for any outdated usages, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to the `Deactivate` function and check if they include the new parameters. # Search for calls to Deactivate in Go files, excluding the function definition itself. rg --type go --glob '!.*/clients.go' 'Deactivate\(' -A 1Length of output: 3361
system/client.go (2)
133-142:
⚠️ Potential issueCheck for nil
clientIDto avoid potential nil pointer dereferenceThe parameter
clientIDis a pointer (*time.ActorID), and callingclientID.String()at line 142 may lead to a nil pointer dereference ifclientIDisnil.Consider adding a nil check for
clientIDbefore using it, or ensure thatclientIDis notnilwhen calling this function.
140-146:
⚠️ Potential issueAvoid including
apiKeyin the shard key to prevent security risksIncluding
apiKeyin the shard key by passing it towithShardKeymay expose sensitive information, posing a security risk. It's recommended to removeapiKeyfrom the shard key and include it in the appropriate authorization header instead.Apply this diff to exclude
apiKeyfrom the shard key and add anAuthorizationheader:- ), apiKey, docKey.String())) + ), docKey.String())) + connectReq := withShardKey(connect.NewRequest(&api.SystemServiceDetachDocumentRequest{ + ProjectId: projectID.String(), + ClientId: clientID.String(), + DocumentId: docID.String(), + DocumentKey: docKey.String(), + }), docKey.String()) + connectReq.Header().Add("Authorization", "Bearer "+apiKey)Committable suggestion was skipped due to low confidence.
56c2073 to
d97f668
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
api/yorkie/v1/system.proto (2)
25-28: LGTM: SystemService is well-defined with a clear purpose.The SystemService definition is concise and includes an appropriate RPC method for detaching documents.
Consider expanding the comment to provide more context:
-// System is a service that provides an API for Cluster. +// SystemService provides API endpoints for cluster-level operations, such as managing document attachments.This gives a clearer indication of the service's scope and functionality.
30-36: LGTM: Request message structure is appropriate for the operation.The
SystemServiceDetachDocumentRequestmessage contains all necessary fields to uniquely identify a document for detachment.Consider enhancing the TODO comment:
- // TODO(hackerwins): Consider to replace this fields with types.Project, types.Client, types.DocumentSummary + // TODO(hackerwins): Replace these fields with types.Project, types.Client, and types.DocumentSummary + // for improved type safety and consistency. Track this in issue #XXXX.This provides more context and links the TODO to a trackable issue.
Would you like me to create a GitHub issue to track this TODO item?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- api/yorkie/v1/system.proto (1 hunks)
- build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1 hunks)
- build/charts/yorkie-cluster/values.yaml (1 hunks)
- server/clients/clients.go (2 hunks)
- test/complex/main_test.go (1 hunks)
- test/integration/housekeeping_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- build/charts/yorkie-cluster/templates/yorkie/deployment.yaml
- build/charts/yorkie-cluster/values.yaml
- test/integration/housekeeping_test.go
🧰 Additional context used
🪛 buf
api/yorkie/v1/system.proto
18-18: Files with package "yorkie.v1" must be within a directory "yorkie/v1" relative to root but were in directory "api/yorkie/v1".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (7)
api/yorkie/v1/system.proto (1)
1-24: LGTM: Copyright, package declaration, and options are well-defined.The copyright notice, syntax declaration, package information, and language-specific options are correctly specified and up-to-date.
Regarding the static analysis hint about package directory mismatch, let's verify the project structure:
This will help us understand if the current structure is consistent across the project or if adjustments are needed.
✅ Verification successful
Verified: Package directory matches declarations.
The project structure aligns with the package declarations, and no discrepancies were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the project structure for proto files # Test: Check if there are other proto files in the same directory echo "Other proto files in api/yorkie/v1:" fd -e proto . api/yorkie/v1 # Test: Check if there's a yorkie/v1 directory with proto files echo "Proto files in yorkie/v1 (if exists):" fd -e proto . yorkie/v1 # Test: Check the overall structure of proto files echo "Overall structure of proto files:" fd -e proto . | sed 's/^/ /'Length of output: 655
🧰 Tools
🪛 buf
18-18: Files with package "yorkie.v1" must be within a directory "yorkie/v1" relative to root but were in directory "api/yorkie/v1".
(PACKAGE_DIRECTORY_MATCH)
server/clients/clients.go (6)
25-27: Import statements are appropriately added.The addition of the
backendandsystempackages is necessary for the new functionalities introduced in the code.
51-54: UpdatedDeactivatefunction signature enhances functionality.The updated function signature now accepts
*backend.Backendand*types.Projectparameters, which allows access to the backend services and project-specific information required for document detachment and client deactivation.
55-59: Proper retrieval of active client information.Using
FindActiveClientInfoensures that only active clients are processed, and appropriate error handling is in place if the client is not found or not active.
60-65: Dialing the system service with dynamic gateway address.The use of
be.Config.GatewayAddrinstead of a hardcoded address enhances flexibility across different deployment environments. Thesystem.WithInsecure(true)option is acceptable if the system operates within a trusted network, but consider using secure communication channels if possible to enhance security.If security is a concern, verify whether the system service supports secure connections and update the dialing options accordingly.
66-85: Efficient document detachment before client deactivation.The loop correctly iterates over the client's documents, checks their status, and detaches them using
systemClient.DetachDocument. Error handling is appropriately managed at each step.Note on the existing TODO about N+1 problem.
There is a TODO comment to solve the N+1 problem when fetching document information. Since this has been noted in previous reviews, ensure that this optimization is addressed in a future update to avoid potential performance issues.
90-95: Client deactivation is performed after successful document detachment.The client is properly deactivated using
be.DB.DeactivateClientafter all associated documents have been detached. Error handling ensures that any issues during deactivation are reported.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
api/docs/yorkie/v1/system.openapi.yaml (2)
12-24: Add a description for the DetachDocument operation.The path for
/yorkie.v1.SystemService/DetachDocumentis well-defined, but it lacks a description. Consider adding a brief explanation of what this operation does to improve API documentation.For example, you could add:
description: "Detaches a document from a client when the client is deactivated."
290-291: Address the TODO comment in SystemServiceDetachDocumentRequest schema.There's a TODO comment suggesting a potential refactoring of the
project,client, anddocumentSummaryfields. Consider creating an issue to track this refactoring task and remove the TODO comment from the OpenAPI specification.system/client.go (3)
61-66: Clarify the struct comment forClientThe comment for the
Clientstruct mentions it is a client for the admin service. Since it interacts with theSystemService, consider updating the comment to reflect this for clarity.Apply this diff:
-// Client is a client for admin service. +// Client is a client for the SystemService. type Client struct { conn *http.Client client v1connect.SystemServiceClient logger *zap.Logger }
125-128: Update the comment for theClosemethodThe comment for the
Closemethod refers to closing the connection to the admin service. For consistency and accuracy, update it to refer to theSystemService.Apply this diff:
-// Close closes the connection to the admin service. +// Close closes the connection to the SystemService. func (c *Client) Close() { c.conn.CloseIdleConnections() }
157-159: Use standard Go comment style for multi-line commentsThe comment for
withShardKeyuses block comment syntax (/** ... */), which is uncommon in Go. It's idiomatic to use//for comments, even for multi-line comments. Consider updating for consistency.Apply this diff:
-/** -* withShardKey returns a context with the given shard key in metadata. - */ +// withShardKey returns a context with the given shard key in metadata.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/yorkie/v1/system.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
- api/docs/yorkie/v1/system.openapi.yaml (1 hunks)
- api/yorkie/v1/system.proto (1 hunks)
- server/clients/clients.go (2 hunks)
- server/rpc/system_server.go (1 hunks)
- system/client.go (1 hunks)
🧰 Additional context used
🪛 buf
api/yorkie/v1/system.proto
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
🔇 Additional comments (11)
api/yorkie/v1/system.proto (3)
28-30: LGTM: Service definition is clear and aligned with PR objectives.The
SystemServicewith theDetachDocumentRPC method is well-defined and matches the PR's goal of detaching documents when a client is deactivated.
32-38: 🛠️ Refactor suggestionConsider adding a status field to the response message.
The
SystemServiceDetachDocumentRequestmessage is well-defined with the necessary fields. However, theSystemServiceDetachDocumentResponsemessage is currently empty. As suggested in a previous review, including a status field or operation result could provide valuable feedback.Consider modifying the response message:
-message SystemServiceDetachDocumentResponse {} +message SystemServiceDetachDocumentResponse { + // Indicates whether the detach operation was successful + bool success = 1; + // Optional message providing additional details about the operation + string message = 2; +}This change would allow the service to communicate the operation's outcome more explicitly.
20-20: Verify the import statement forresources.proto.The static analysis tool indicates that the file
yorkie/v1/resources.protodoes not exist. Please ensure that this file is present in the correct location within your project structure.✅ Verification successful
Import statement for
resources.protois valid and the file exists.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the imported proto file # Test: Check if the resources.proto file exists fd -t f resources.protoLength of output: 55
🧰 Tools
🪛 buf
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
api/docs/yorkie/v1/system.openapi.yaml (5)
1-11: LGTM: API Information and Servers section is well-defined.The OpenAPI version, API information, and server definitions are clear and appropriate. Good job on providing both production and local server environments.
25-300: LGTM: Components section is well-structured and comprehensive.The components section, including request bodies, responses, and schemas, is well-defined and uses proper references. The use of
google.protobuf.Timestampfor date-time fields is a good practice. The schemas are detailed and cover all necessary aspects of the API.
301-307: LGTM: Security scheme is well-defined.The API key authentication scheme is properly defined and applied globally. This ensures that all endpoints are secured by default, which is a good security practice.
308-310: LGTM: Tags section is concise and informative.The tag for
yorkie.v1.SystemServiceis well-defined and includes a helpful description of the service's purpose.
1-310: Overall, excellent OpenAPI specification for the Yorkie API.This OpenAPI 3.1.0 specification is well-structured, comprehensive, and follows best practices. It provides clear definitions for the API's endpoints, request/response schemas, and security requirements. The use of references and appropriate data types enhances maintainability and clarity.
A few minor improvements were suggested:
- Adding a description for the DetachDocument operation.
- Addressing the TODO comment in the SystemServiceDetachDocumentRequest schema.
These small enhancements will further improve the already high-quality specification.
server/rpc/system_server.go (3)
38-47: Initialization ofsystemServeris appropriateThe
systemServerstruct is correctly initialized with the provided backend in thenewSystemServerfunction, ensuring the server has access to necessary backend services.
71-73: Handle locker unlock errors appropriatelyCurrently, if unlocking the locker fails, the error is logged. Consider whether additional error handling is needed, such as retrying the unlock or propagating the error if it could have significant implications.
76-82: Check for client deactivation statusEnsure that the client retrieved is active before proceeding. If the client is already deactivated, attempting to detach documents may not be necessary and could lead to errors.
Run the following script to verify the client's status:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1036 +/- ##
==========================================
- Coverage 48.76% 48.30% -0.46%
==========================================
Files 79 80 +1
Lines 11343 11415 +72
==========================================
- Hits 5531 5514 -17
- Misses 5269 5358 +89
Partials 543 543 ☔ View full report in Codecov by Sentry. |
a1b1283 to
e0753b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- server/clients/clients.go (2 hunks)
- server/rpc/server.go (1 hunks)
- system/client.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/rpc/server.go
🧰 Additional context used
🔇 Additional comments (6)
server/clients/clients.go (4)
25-27: LGTMThe newly added import statements are appropriate and necessary for the updated functionality.
51-56: Updated function signature reflects necessary changesThe
Deactivatefunction now acceptsbe *backend.Backendandproject *types.Projectas parameters instead of justdb database.Database. This change provides access to the backend services and project information required for detaching documents properly.
71-78: Address the N+1 query problem in the document detachment loopThe current implementation fetches
docInfoindividually for each document usingFindDocInfoByRefKey. This can lead to performance issues due to multiple database calls when a client has many documents attached. The TODO comment on line 71 acknowledges this issue.
90-95: LGTMClient deactivation is handled correctly, and errors are appropriately managed.
system/client.go (2)
62-95: Implementation ofClientstruct andNewfunction looks goodThe
Clientstruct encapsulates the necessary components for the admin service client, and theNewfunction correctly initializes the client with configurable options.
140-150:⚠️ Potential issueFix misaligned parentheses in
DetachDocumentmethodThere appears to be a misalignment of parentheses in the call to
c.client.DetachDocument, which could lead to a syntax error during compilation.Apply this diff to correct the syntax:
func (c *Client) DetachDocument( ctx context.Context, project *types.Project, clientID *time.ActorID, docID types.ID, apiKey string, docKey key.Key, ) error { _, err := c.client.DetachDocument( ctx, - withShardKey(connect.NewRequest(&api.SystemServiceDetachDocumentRequest{ + withShardKey( + connect.NewRequest(&api.SystemServiceDetachDocumentRequest{ Project: converter.ToProject(project), ClientId: clientID.String(), DocumentSummary: converter.ToDocumentSummary(&types.DocumentSummary{ ID: docID, Key: docKey, }), - }, - ), apiKey, docKey.String())) + }), + apiKey, docKey.String(), + ), + ) if err != nil { return err } return nil }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
api/yorkie/v1/cluster.proto (1)
32-38: Consider adding a status field to the response message.The message definitions for
ClusterServiceDetachDocumentRequestandClusterServiceDetachDocumentResponseare well-structured. The request message contains all necessary fields for the detach operation.However, consider adding a status field to the
ClusterServiceDetachDocumentResponseto provide explicit feedback on the operation's success or failure. This could be implemented as follows:-message ClusterServiceDetachDocumentResponse {} +message ClusterServiceDetachDocumentResponse { + bool success = 1; + string message = 2; // Optional: for additional details or error messages +}This change would provide more informative responses to the client, improving error handling and debugging capabilities.
api/docs/yorkie/v1/cluster.openapi.yaml (3)
1-11: LGTM! Consider adding contact information.The OpenAPI metadata and server definitions are well-structured and provide essential information about the API. Good job on including both production and local development server URLs.
Consider adding a
contactfield in theinfosection to provide information on how to reach the API maintainers. This can be helpful for users who need support or want to report issues. For example:info: # ... existing fields ... contact: name: Yorkie Team url: https://yorkie.dev/contact email: support@yorkie.dev
12-24: Add summary and description to the endpoint.The endpoint definition is structured well, but it lacks a summary and description. Adding these would improve the API documentation and make it more user-friendly.
Consider adding a summary and description to the endpoint like this:
/yorkie.v1.ClusterService/DetachDocument: post: summary: Detach a document from a client description: | This endpoint detaches a document from a specified client in the cluster. It's typically used when a client is deactivated or no longer needs access to the document. # ... rest of the existing definition ...Also, consider adding
operationIdfor easier reference in generated client code:operationId: detachDocument
300-309: Security scheme is good, but could be more specific.The API key authentication scheme is well-defined and appropriately applied globally. This ensures all endpoints are protected by default.
Consider specifying the expected format of the API key in the security scheme description. This can help API consumers understand how to properly construct the Authorization header. For example:
securitySchemes: ApiKeyAuth: type: apiKey in: header name: Authorization description: "API key authentication. Use the format: 'Bearer <API_KEY>'"This addition clarifies that the API key should be prefixed with "Bearer " in the Authorization header, which is a common convention for token-based authentication.
client/client.go (1)
Line range hint
1-1000: Overall impact of the protocol selection changeThe modification to the
Dialmethod improves the security and clarity of the protocol selection process. However, given the critical nature of theClientstruct in establishing connections, please ensure the following:
- Comprehensive testing of the
Dialmethod with various configurations (with and without certificate files) to verify correct behavior.- Review and update of any client-side documentation or examples that demonstrate connection establishment.
- Consideration of backward compatibility, especially if any existing clients rely on the previous behavior.
Consider adding a configuration option to explicitly set the desired protocol (HTTP/HTTPS) in addition to the certificate-based inference. This would provide more flexibility and control to the clients while maintaining the security improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/yorkie/v1/cluster.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
- admin/client.go (3 hunks)
- api/docs/yorkie/v1/cluster.openapi.yaml (1 hunks)
- api/yorkie/v1/cluster.proto (1 hunks)
- api/yorkie/v1/v1connect/cluster.connect.go (1 hunks)
- client/client.go (1 hunks)
- cluster/client.go (1 hunks)
- server/clients/clients.go (2 hunks)
- server/rpc/cluster_server.go (1 hunks)
- server/rpc/server.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/rpc/server.go
🧰 Additional context used
🪛 buf
api/yorkie/v1/cluster.proto
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
🔇 Additional comments (15)
api/yorkie/v1/cluster.proto (2)
27-30: LGTM: Service definition is clear and consistent.The
ClusterServicedefinition with theDetachDocumentRPC method is well-structured and aligns with the PR objectives. The naming convention follows the standard protobuf style, and the method signature is appropriate for the intended functionality.
1-26: Verify the existence of the imported file.The file header, syntax declaration, package declaration, and language-specific options look good. However, there's a potential issue with the imported file.
Please run the following script to check if the imported file exists:
If the file doesn't exist, please ensure it's created or update the import statement accordingly.
🧰 Tools
🪛 buf
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
admin/client.go (4)
75-75: LGTM: Addition ofisInsecurefield enhances clarity.The new
isInsecurefield in the Client struct centralizes the insecure connection logic, improving the overall design and maintainability of the code.
104-104: LGTM: Proper initialization ofisInsecurefield.The
isInsecurefield is correctly initialized in theNewfunction, ensuring consistency with the Options struct and proper setup of the Client instance.
Line range hint
125-129: LGTM: Improved protocol selection based onisInsecurefield.The
Dialmethod now correctly uses theisInsecurefield to determine the appropriate protocol (http or https). This change centralizes the insecure connection logic and improves code organization.
Line range hint
75-129: Summary: Successful implementation ofisInsecurefeature.The changes introduce an
isInsecurefield to the Client struct and update theNewandDialmethods accordingly. This implementation centralizes the insecure connection logic, enhancing code clarity and maintainability. The changes are consistent and well-integrated throughout the file.api/docs/yorkie/v1/cluster.openapi.yaml (1)
1-309: Overall, excellent API specification with room for minor enhancements.This OpenAPI 3.1.0 specification for the Yorkie ClusterService is well-structured and comprehensive. It provides clear definitions for endpoints, request/response schemas, and security requirements. The use of reusable components and standard formats (like Google's protobuf Timestamp) demonstrates good API design practices.
To further improve this already solid specification, consider implementing the following suggestions:
- Add contact information in the API metadata.
- Enhance the endpoint documentation with summaries, descriptions, and operation IDs.
- Clarify empty response schemas and improve descriptions for certain fields (e.g., clientDeactivateThreshold, secretKey).
- Specify the expected format for the API key in the security scheme description.
These enhancements will make the API even more user-friendly and easier to integrate with.
client/client.go (1)
Line range hint
196-200: Improved security and clarity in protocol selectionThe change in determining whether to use HTTP or HTTPS based on the presence of a certificate file (
c.options.CertFile) is a good improvement. It provides a more direct and secure approach to protocol selection.However, please consider the following points:
- Ensure that all client configurations are updated to set the
CertFileoption correctly if HTTPS is required.- Update any documentation or client setup guides to reflect this change in behavior.
- Consider adding a log statement (debug level) to indicate which protocol is being used, for easier troubleshooting.
To ensure this change doesn't introduce any regressions, please run the following verification:
server/clients/clients.go (3)
25-26: Import ofclusterpackage is appropriateThe addition of the
clusterpackage is necessary for dialing the cluster service usingcluster.Dial. No issues found.
60-67: Dynamic gateway address improves flexibilityReplacing the hardcoded system service address with
be.Config.GatewayAddrenhances configurability and supports different deployment environments.
73-81: Previous comment on N+1 query issue still applicableThe code still fetches
docInfoindividually within the loop, leading to potential performance issues due to multiple database calls. TheTODOcomment acknowledges this issue.server/rpc/cluster_server.go (2)
73-76: Ensure proper handling of unlock errorsCurrently, if an error occurs during
locker.Unlock(ctx), it is logged but not propagated. Depending on the criticality, consider whether additional handling is necessary, such as retrying the unlock or returning the error to the caller to prevent potential resource leaks or deadlocks.
101-104: Verify that clearing presence detaches the document correctlyIn the
doc.Updatefunction, onlyp.Clear()is called to detach the document. Please confirm that this action sufficiently detaches the document and that no additional steps are required, such as updating document status or notifying other components.cluster/client.go (1)
142-152:⚠️ Potential issueEnsure
apiKeyis not unintentionally included in the shard keyIn the
DetachDocumentmethod, theapiKeyis being passed to thewithShardKeyfunction along withdocKey.String(). Including theapiKeyin the shard key might not be appropriate and could raise security concerns if the shard key is logged or exposed.Consider modifying the code to handle the
apiKeyseparately, possibly by adding it to the request headers as an authorization token.Apply this diff to remove
apiKeyfrom the shard key:}), - apiKey, docKey.String())) + docKey.String())) + // Add the apiKey to the request header as an authorization token. + req := withShardKey(connect.NewRequest(...), docKey.String()) + req.Header().Add("Authorization", fmt.Sprintf("Bearer %s", apiKey)) + _, err := c.client.DetachDocument(ctx, req)Run the following script to verify how
apiKeyshould be handled in similar contexts:#!/bin/bash # Description: Check how apiKey is managed in requests across the codebase. # Find all occurrences where apiKey is set in request headers. rg --type go 'Header\(\)\.Add\("Authorization",.*apiKey.*\)' # Search for usage patterns of withShardKey without apiKey. rg --type go 'withShardKey\(.*docKey\.String\(\)\)'api/yorkie/v1/v1connect/cluster.connect.go (1)
1-120: LGTM!The new
ClusterServiceclient and handler code are correctly implemented and align with the service definitions. The generated code follows best practices and maintains consistency across the service interfaces.
15f7e2e to
9372629
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
api/yorkie/v1/cluster.proto (1)
32-38: LGTM: Message definitions are well-structured. Consider adding a status field to the response.The
ClusterServiceDetachDocumentRequestmessage is well-defined with appropriate fields for the detach operation. The emptyClusterServiceDetachDocumentResponseis acceptable if no data needs to be returned.Consider adding a status field to the response message to explicitly indicate the success or failure of the operation. For example:
-message ClusterServiceDetachDocumentResponse {} +message ClusterServiceDetachDocumentResponse { + bool success = 1; + string message = 2; // Optional: for additional information or error messages +}This change would provide more explicit feedback about the operation's outcome.
api/docs/yorkie/v1/cluster.openapi.yaml (3)
12-24: Add a description for the DetachDocument endpoint.The endpoint definition is correct, but it lacks a description. Consider adding a brief explanation of the endpoint's purpose to improve API documentation.
For example:
description: "Detaches a document from a client in the cluster."
178-205: Enhance schema documentation and clarify empty response.
- Add descriptions to the
ClusterServiceDetachDocumentRequestschema properties to improve documentation.- The
ClusterServiceDetachDocumentResponseschema is empty. If this is intentional (i.e., the operation doesn't return any data), add a description to clarify this. Otherwise, add the expected properties to the schema.Example improvements:
yorkie.v1.ClusterServiceDetachDocumentRequest: properties: clientId: description: "Unique identifier of the client detaching the document." documentSummary: description: "Summary information of the document being detached." project: description: "Project information related to the document detachment." yorkie.v1.ClusterServiceDetachDocumentResponse: description: "Response object for the DetachDocument operation. This response is intentionally empty as the operation does not return any data."
261-265: ImproveclientDeactivateThresholdfield definition.The
clientDeactivateThresholdfield in theProjectschema is defined as a string without a specific format or description. Consider using a more specific format and adding a description to clarify its purpose and expected value.Suggested improvement:
clientDeactivateThreshold: type: string format: duration description: "The threshold duration for client deactivation, in ISO 8601 duration format (e.g., 'PT15M' for 15 minutes)."server/clients/clients.go (1)
Line range hint
45-45: Update the function comment to reflect the new logicThe
Deactivatefunction now detaches documents before deactivating the client. Consider updating the function's documentation comment to reflect this change for better clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/yorkie/v1/cluster.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
- admin/client.go (3 hunks)
- api/docs/yorkie/v1/cluster.openapi.yaml (1 hunks)
- api/yorkie/v1/cluster.proto (1 hunks)
- api/yorkie/v1/v1connect/cluster.connect.go (1 hunks)
- client/client.go (1 hunks)
- cluster/client.go (1 hunks)
- server/clients/clients.go (2 hunks)
- server/rpc/cluster_server.go (1 hunks)
- server/rpc/server.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- admin/client.go
- api/yorkie/v1/v1connect/cluster.connect.go
- client/client.go
- cluster/client.go
- server/rpc/cluster_server.go
- server/rpc/server.go
🧰 Additional context used
🪛 buf
api/yorkie/v1/cluster.proto
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
🔇 Additional comments (7)
api/yorkie/v1/cluster.proto (2)
27-30: LGTM: Service definition is clear and aligns with PR objectives.The
ClusterServicedefinition with theDetachDocumentRPC method is well-structured and follows proper naming conventions. It accurately represents the functionality described in the PR objectives.
20-20: Verify the import statement and ensure the referenced file exists.The static analysis tool indicates that the file "yorkie/v1/resources.proto" does not exist. Please verify if this file is present in the correct location within the project structure.
To verify the existence of the file, you can run the following command:
If the file is missing, consider creating it or updating the import statement. Would you like assistance in resolving this issue?
🧰 Tools
🪛 buf
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
api/docs/yorkie/v1/cluster.openapi.yaml (3)
1-11: LGTM: Well-structured OpenAPI metadata and server information.The OpenAPI version, API information, and server URLs are correctly defined and provide clear information about the Yorkie API.
300-304: LGTM: Well-defined API key authentication scheme.The security scheme for API key authentication is correctly defined, specifying the use of the Authorization header.
305-309: LGTM: Clear and concise tag definition for ClusterService.The tag for the ClusterService is well-defined, including a clear description of its purpose.
server/clients/clients.go (2)
48-49: Align parameter usage betweenActivateandDeactivatefunctionsThe
Deactivatefunction now acceptsbe *backend.Backendandproject *types.Project, whereas theActivatefunction acceptsdb database.Databaseandproject *types.Project. For consistency and better maintainability, consider updating theActivatefunction to also usebe *backend.Backend.
73-81: Address the N+1 query problem in the document detachment loopThe current implementation fetches
docInfoindividually for each document within the loop usingFindDocInfoByRefKey. This can lead to performance issues due to multiple database calls, especially when a client has many documents attached. The TODO comment on line 73 acknowledges this issue.Consider fetching all required
DocInforecords in a single batch operation before the loop. You could implement a method likeFindDocInfosByRefKeysthat accepts a slice ofDocRefKeys and returns the correspondingDocInfoobjects. This change would reduce the number of database queries and improve performance.
9372629 to
daf0b91
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
api/yorkie/v1/cluster.proto (2)
27-30: LGTM: Service definition is clear and aligned with PR objectives.The
ClusterServicewith itsDetachDocumentRPC method is well-defined and consistent with the goal of detaching documents when a client is deactivated.Consider adding a brief comment above the
DetachDocumentRPC method to describe its purpose and any important details about its usage. For example:// DetachDocument detaches a document from a client in the cluster. // This is typically called when a client is deactivated. rpc DetachDocument (ClusterServiceDetachDocumentRequest) returns (ClusterServiceDetachDocumentResponse) {}
32-38: LGTM: Message definitions are appropriate for the DetachDocument operation.The
ClusterServiceDetachDocumentRequestmessage includes all necessary fields for identifying the project, client, and document to be detached. The emptyClusterServiceDetachDocumentResponseis suitable for this operation.For consistency with other parts of the Yorkie API, consider renaming the messages to remove the service name prefix:
message DetachDocumentRequest { // ... (keep the existing fields) } message DetachDocumentResponse {}This change would make the messages more reusable if needed in other contexts and aligns with common gRPC naming conventions.
cluster/client.go (1)
85-90: Enhance error context when creating the default loggerWhen creating the default production logger, consider adding more context to the error message. This will aid in debugging and understanding the source of the failure.
Apply this diff to wrap the error with additional context:
if logger == nil { l, err := zap.NewProduction() if err != nil { - return nil, fmt.Errorf("create logger: %w", err) + return nil, fmt.Errorf("create default production logger for cluster client: %w", err) } logger = l }server/packs/packs_test.go (1)
Line range hint
234-309: Address skipped test case.This test case is currently skipped due to an unresolved pushpull consistency problem. The test appears to be checking important edge cases related to change duplication and client info updates, which are crucial for the system's reliability.
To ensure this doesn't get overlooked:
- Create an issue to track the resolution of the pushpull consistency problem.
- Add a TODO comment in the code referencing the created issue.
- Once resolved, remove the
t.Skipline and re-enable the test.Would you like me to create a GitHub issue for tracking this task?
api/docs/yorkie/v1/cluster.openapi.yaml (2)
12-24: Add a description for the DetachDocument endpoint.The endpoint is well-defined, but it lacks a description. Consider adding a brief explanation of the endpoint's purpose to improve API documentation.
For example:
description: "Detaches a document from a client in the cluster."
178-200: Add descriptions to the ClusterServiceDetachDocumentRequest schema properties.Consider adding descriptions to the
clientId,documentSummary, andprojectproperties to provide more context about their purpose and usage.For example:
clientId: type: string description: "Unique identifier of the client detaching the document."server/clients/clients.go (2)
60-61: Address the TODO: Introduce cluster client poolThe current implementation creates a new cluster client connection for each deactivation. Introducing a cluster client pool, as mentioned in the TODO, would improve performance by reusing connections and reducing overhead.
Would you like assistance in implementing a cluster client pool? I can help provide a proposed solution or open a GitHub issue to track this task.
41-92: Update unit tests to cover changes in activation and deactivationWith the modifications to
ActivateandDeactivatefunctions, including new parameters and logic changes, please ensure that unit tests are updated or added to cover these changes.Proper testing helps prevent regressions and ensures the new code behaves as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/yorkie/v1/cluster.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
- admin/client.go (3 hunks)
- api/docs/yorkie/v1/cluster.openapi.yaml (1 hunks)
- api/yorkie/v1/cluster.proto (1 hunks)
- api/yorkie/v1/v1connect/cluster.connect.go (1 hunks)
- client/client.go (1 hunks)
- cluster/client.go (1 hunks)
- server/clients/clients.go (2 hunks)
- server/packs/packs_test.go (3 hunks)
- server/rpc/cluster_server.go (1 hunks)
- server/rpc/server.go (1 hunks)
- server/rpc/yorkie_server.go (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- admin/client.go
- api/yorkie/v1/v1connect/cluster.connect.go
- client/client.go
- server/rpc/cluster_server.go
- server/rpc/server.go
- server/rpc/yorkie_server.go
🧰 Additional context used
🪛 buf
api/yorkie/v1/cluster.proto
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
🔇 Additional comments (17)
api/yorkie/v1/cluster.proto (2)
1-26: LGTM: File header and metadata are well-structured.The file header includes the appropriate license information, and the metadata (syntax, package, and options) is correctly defined for both Go and Java implementations.
🧰 Tools
🪛 buf
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
20-20: Verify the existence and location of the imported resources.proto file.The static analysis tool reports that the file "yorkie/v1/resources.proto" does not exist. This import is crucial as it likely contains definitions for the
ProjectandDocumentSummarytypes used in theClusterServiceDetachDocumentRequestmessage.Please run the following script to check for the existence of the resources.proto file and its contents:
If the file is not found or doesn't contain the expected message definitions, please update the import statement or create the missing file with the necessary definitions.
✅ Verification successful
Import statement is valid. The
resources.protofile exists and contains the required message definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the resources.proto file # Test 1: Check if the file exists if fd -t f "resources.proto" | grep -q .; then echo "resources.proto file found" # Test 2: Check the content of the file fd -t f "resources.proto" -x cat {} \; | grep -E "message (Project|DocumentSummary)" else echo "resources.proto file not found" # Test 3: Search for alternative locations or names fd -t f -e proto | xargs grep -l -E "message (Project|DocumentSummary)" fiLength of output: 232
🧰 Tools
🪛 buf
20-20: import "yorkie/v1/resources.proto": file does not exist
(COMPILE)
cluster/client.go (7)
44-46: LGTM: Well-implemented option functionThe
WithLoggerfunction is correctly implemented as an option function, following the functional options pattern. This is a good practice for configurable structs in Go.
49-51: LGTM: Well-implemented option functionThe
WithSecurefunction is correctly implemented as an option function, following the functional options pattern. The function name and parameter are clear and intuitive.
99-111: LGTM: Well-implemented Dial functionThe
Dialfunction correctly creates a new Client instance and dials to the admin service. Error handling is implemented properly, and the function follows a clear and logical flow.
114-126: LGTM: Well-implemented Dial methodThe
Dialmethod correctly handles the RPC address, prepending the appropriate scheme based on theisSecureflag. The client initialization is done properly with the RPC address.
129-131: LGTM: Proper resource cleanupThe
Closemethod correctly closes idle connections, which is a good practice for cleaning up resources when the client is no longer needed.
163-167: LGTM: Well-implemented generic helper functionThe
withShardKeyfunction is a well-implemented generic helper that correctly adds the shard key to the request header. It's flexible enough to handle multiple keys and works with different request types.
1-167: LGTM: Well-structured and organized packageThe
clusterpackage is well-organized with a clear separation of concerns. It follows Go best practices for package structure and naming conventions. The use of the functional options pattern for client configuration is a good practice, making the client flexible and easy to use.The code is generally well-written, maintainable, and makes good use of Go idioms and patterns.
server/packs/packs_test.go (3)
280-283: LGTM. Consistent change applied.The update to use
testBackendinstead oftestBackend.DBin theclients.FindActiveClientInfocall is consistent with the previous change. This demonstrates a uniform application of the modification throughout the test file.
306-309: LGTM. Verify all instances are updated.The consistent update to use
testBackendinstead oftestBackend.DBin theclients.FindActiveClientInfocall is approved. To ensure completeness, it's advisable to verify that all instances in the file have been updated.Run the following command to check for any remaining instances of
FindActiveClientInfothat might still be usingtestBackend.DB:#!/bin/bash # Description: Check for any remaining instances of FindActiveClientInfo using testBackend.DB # Test: Search for FindActiveClientInfo calls still using testBackend.DB rg --type go 'clients\.FindActiveClientInfo\(.*testBackend\.DB' server/packs/packs_test.go
234-237: LGTM. Verify FindActiveClientInfo method signature.The change from
testBackend.DBtotestBackendin theclients.FindActiveClientInfocall is consistent with the expected updates. This likely reflects a change in the method signature in theclientspackage.To ensure consistency across the codebase, please run the following command to verify the
FindActiveClientInfomethod signature:api/docs/yorkie/v1/cluster.openapi.yaml (3)
1-11: LGTM: Well-structured OpenAPI metadata and server information.The OpenAPI version, API information, and server URLs are correctly defined and provide clear information about the API and its environments.
300-304: LGTM: Well-defined API key authentication scheme.The security scheme for API key authentication is correctly defined and follows best practices by using the Authorization header.
305-309: LGTM: Clear and concise tag definition for ClusterService.The tag for ClusterService is well-defined and includes a helpful description of the service's purpose.
server/clients/clients.go (2)
41-45: UpdatedActivatefunction signature improves consistencyThe
Activatefunction now acceptsbe *backend.Backendandproject *types.Project, aligning it with theDeactivatefunction. This enhances consistency and maintainability across the API.
98-101: Ensure consistent parameter usage inFindActiveClientInfoThe
FindActiveClientInfofunction now acceptsbe *backend.Backend, aligning with other functions. Ensure that all calls to this function are updated accordingly throughout the codebase.Run the following script to find all usages of
FindActiveClientInfoand verify they pass the correct parameters:✅ Verification successful
All usages of
FindActiveClientInfohave been consistently updated with thebe *backend.Backendparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of FindActiveClientInfo have been updated. # Search for function calls to FindActiveClientInfo rg --type go 'FindActiveClientInfo\(' -A 2Length of output: 2756
| for docID, clientDocInfo := range info.Documents { | ||
| if clientDocInfo.Status != database.DocumentAttached { | ||
| continue | ||
| } | ||
|
|
||
| // TODO(hackerwins): Solve N+1 | ||
| docInfo, err := be.DB.FindDocInfoByRefKey(ctx, types.DocRefKey{ | ||
| ProjectID: project.ID, | ||
| DocID: docID, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| actorID, err := info.ID.ToActorID() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := cli.DetachDocument(ctx, project, actorID, docID, project.PublicKey, docInfo.Key); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling partial failures during document detachment
If an error occurs while detaching a document, the function returns immediately, and the client remains active. This means that a failure in detaching one document prevents the deactivation of the client.
Consider modifying the error handling to ensure the client is deactivated even if some document detachment operations fail. You can collect the errors and proceed with deactivation:
var detachmentErrors []error
for docID, clientDocInfo := range info.Documents {
if clientDocInfo.Status != database.DocumentAttached {
continue
}
docInfo, err := be.DB.FindDocInfoByRefKey(ctx, types.DocRefKey{
ProjectID: project.ID,
DocID: docID,
})
if err != nil {
detachmentErrors = append(detachmentErrors, err)
continue
}
actorID, err := info.ID.ToActorID()
if err != nil {
detachmentErrors = append(detachmentErrors, err)
continue
}
if err := cli.DetachDocument(ctx, project, actorID, docID, project.PublicKey, docInfo.Key); err != nil {
detachmentErrors = append(detachmentErrors, err)
continue
}
}
deactivatedClient, err := be.DB.DeactivateClient(ctx, refKey)
if err != nil {
return nil, err
}
if len(detachmentErrors) > 0 {
// Handle or log the errors as needed
// For example, you could wrap them into a single error
return deactivatedClient, fmt.Errorf("client deactivated with errors in detaching documents: %v", detachmentErrors)
}
return deactivatedClient, nilThis approach ensures the client is deactivated regardless of individual detachment failures, and errors are properly logged or handled.
| // TODO(hackerwins): Solve N+1 | ||
| docInfo, err := be.DB.FindDocInfoByRefKey(ctx, types.DocRefKey{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the TODO: Solve N+1 query problem
The current implementation fetches docInfo individually within the loop using FindDocInfoByRefKey, which can lead to performance issues due to multiple database calls.
Consider fetching all required DocInfo records in a single batch operation before the loop. Implementing a method like FindDocInfosByRefKeys would reduce the number of database queries:
docRefKeys := make([]types.DocRefKey, 0, len(info.Documents))
for docID, clientDocInfo := range info.Documents {
if clientDocInfo.Status != database.DocumentAttached {
continue
}
docRefKeys = append(docRefKeys, types.DocRefKey{
ProjectID: project.ID,
DocID: docID,
})
}
docInfos, err := be.DB.FindDocInfosByRefKeys(ctx, docRefKeys)
if err != nil {
return nil, err
}
for _, docInfo := range docInfos {
// Proceed with detachment using docInfo
}This change would optimize database access and improve performance.
This commit implements the detachment of documents attached to clients upon client deactivation. Previously, the status of the document was only changed to 'detach' within DB.clients.ClientDocInfo.Status, leading to issues where the deactivated client's presence remained active and presence change events were not communicated to other clients. With this change, on client deactivation, the document's packs.PushPull method is called directly to execute the complete detachment process. Moreover, since the Yorkie cluster sharding mechanism assigns specific documents to specific servers, the server managing client deactivation may not always be the one handling the document. Therefore, in such cases, a request will be made to the server currently responsible for the document to detach it properly. To facilitate this communication across the cluster, a new ClusterService has been added. Follow-up tasks that need to be addressed after this PR include: - Implementing authentication for cluster communication and preventing external exposure. - Eliminating redundant database resource requests between clients.Deactivate and server.DetachDocument. - Improving the p.Clear ChangePack creation logic. - Introducing a connection pool for clusterClient. --------- Co-authored-by: raararaara <raararaara@gmail.com>
) This change introduces a migration script to handle documents that remain attached to deactivated clients from before PRs #907 and #1036. Instead of direct detachment, the script temporarily reactivates clients to leverage the existing housekeeping process. The approach: - Identifies documents still attached to deactivated clients - Updates client states from deactivated to activated - Lets housekeeping process handle document detachment naturally
What this PR does / why we need it:
This PR implements the detachment of documents attached to clients upon client deactivation. Previously, the status of the document was only changed to 'detach' within
DB.clients.ClientDocInfo.Status, leading to issues where the deactivated client's presence remained active and presence change events were not communicated to other clients.With this change, on client deactivation, the document's packs.PushPull method is called directly to execute the complete detachment process.
Moreover, since the Yorkie cluster sharding mechanism assigns specific documents to specific servers, the server managing client deactivation may not always be the one handling the document. Therefore, in such cases, a request will be made to the server currently responsible for the document to detach it properly. To facilitate this communication across the cluster, a new ClusterService has been added.
Follow-up tasks that need to be addressed after this PR include:
Which issue(s) this PR fixes:
Fixes #602
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation