Replace MaxCreatedAtMapByActor with VersionVector #1088
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing the handling of timestamps and version vectors in operations related to CRDTs (Conflict-free Replicated Data Types). Key changes include updating method signatures to incorporate new parameters, particularly Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1088 +/- ##
==========================================
- Coverage 46.99% 46.89% -0.10%
==========================================
Files 84 84
Lines 12162 12243 +81
==========================================
+ Hits 5715 5741 +26
- Misses 5875 5926 +51
- Partials 572 576 +4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
pkg/document/operations/tree_style.go (2)
54-59: Consider Consistent Field Initialization OrderIn the
NewTreeStyleconstructor, the fields are initialized in a different order than they are declared in the struct. For better readability and maintainability, consider initializing the fields in the same order as their declaration.
72-77: Ensure Field Initialization Order ConsistencySimilarly, in the
NewTreeStyleRemoveconstructor, aligning the field initialization order with the struct declaration can enhance code clarity.pkg/document/operations/tree_edit.go (1)
56-61: Align Field Initialization with Struct DeclarationIn the
NewTreeEditconstructor, the order of field initialization differs from the struct's field declaration. Maintaining consistency enhances readability.pkg/document/crdt/text.go (1)
275-276: Update Method Documentation for Edit FunctionThe signature of the
Editmethod has changed with the addition ofversionVector. Update the method's documentation to reflect this change for clarity.pkg/document/crdt/rga_tree_split.go (1)
453-454: Update Comments to Reflect Signature ChangesThe
editmethod's signature has changed by replacingmaxCreatedAtMapByActorwithversionVector. Ensure that all comments and documentation within the method are updated accordingly.pkg/document/operations/move.go (1)
55-55: Document the Option parameter usageThe added
Optionparameter is currently unused. Consider adding a comment explaining its purpose for future implementation or document why it's required for interface compatibility.pkg/document/operations/set.go (1)
56-56: Consider future version vector integrationThe added
Optionparameter will likely be used for version vector integration. Consider adding a TODO comment or documentation explaining how this parameter will be utilized for version control in future implementations.pkg/document/operations/add.go (1)
55-56: Approve consistent interface changes across operationsThe consistent addition of the
Optionparameter across all operation types (Move,Set, andAdd) provides a clean way to integrate version vector support while maintaining backward compatibility. This change aligns well with the PR objective of replacing maxCreatedAtMap with version vector for causal-concurrent operations.Consider documenting the migration strategy from maxCreatedAtMap to version vector in the package documentation to help maintainers understand the transition.
pkg/document/operations/array_set.go (1)
55-55: LGTM! Consider documenting the Option parameter.The addition of the variadic Option parameter aligns with the standardization of operation execution interfaces. While currently unused, it provides future extensibility.
Consider adding a comment explaining the purpose and expected usage of the Option parameter for future maintainers.
pkg/document/change/change.go (1)
57-57: LGTM! Consistent propagation of version vector to operationsThe change ensures that all operations within a change have access to the version vector from the change ID, which is essential for maintaining causal consistency across concurrent operations.
This modification establishes a clear chain of responsibility for version tracking:
- Change ID maintains the version vector
- Change propagates it to all operations
- Operations use it for causal consistency checks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
api/converter/from_bytes.go(1 hunks)api/converter/from_pb.go(0 hunks)api/converter/to_pb.go(2 hunks)pkg/document/change/change.go(1 hunks)pkg/document/crdt/rga_tree_split.go(7 hunks)pkg/document/crdt/root_test.go(4 hunks)pkg/document/crdt/text.go(3 hunks)pkg/document/crdt/text_test.go(2 hunks)pkg/document/crdt/tree.go(13 hunks)pkg/document/crdt/tree_test.go(1 hunks)pkg/document/json/text.go(2 hunks)pkg/document/json/tree.go(3 hunks)pkg/document/operations/add.go(1 hunks)pkg/document/operations/array_set.go(1 hunks)pkg/document/operations/edit.go(1 hunks)pkg/document/operations/increase.go(1 hunks)pkg/document/operations/move.go(1 hunks)pkg/document/operations/operation.go(1 hunks)pkg/document/operations/remove.go(1 hunks)pkg/document/operations/set.go(1 hunks)pkg/document/operations/style.go(1 hunks)pkg/document/operations/tree_edit.go(3 hunks)pkg/document/operations/tree_style.go(3 hunks)pkg/document/time/version_vector.go(1 hunks)
💤 Files with no reviewable changes (1)
- api/converter/from_pb.go
👮 Files not reviewed due to content moderation or server errors (3)
- pkg/document/json/tree.go
- api/converter/to_pb.go
- pkg/document/crdt/tree_test.go
🔇 Additional comments (21)
pkg/document/operations/tree_style.go (2)
97-98:
Handle Nil VersionVector Gracefully
When calling obj.Style and obj.RemoveStyle, options.VersionVector is passed, which may be nil. Ensure that these methods handle a nil VersionVector without causing runtime errors.
Run the following script to verify handling of nil VersionVector:
82-87: Verify Proper Option Handling in Execute Method
The Execute method now accepts variadic options opts ...Option. Ensure that all options are correctly applied and that default values are set appropriately when options are not provided.
Run the following script to check the usage of ExecuteOption:
✅ Verification successful
Let me gather more information about how the ExecuteOption is used in the TreeStyle operation.
Let me check one more aspect to see how the version vector is used in the operation.
Option Handling in Execute Method is Properly Implemented
The implementation correctly handles the options pattern. The ExecuteOption is properly initialized and applied, and its VersionVector field is correctly utilized in both Style and RemoveStyle operations. The code follows the established pattern where options are applied through the variadic parameter, and default values are maintained when no options are provided.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ExecuteOption is correctly utilized in Execute methods.
# Search for ExecuteOption struct definition and its field usages.
rg 'type ExecuteOption struct' -A 10
Length of output: 878
Script:
#!/bin/bash
# Search for usage of ExecuteOption in TreeStyle operation
rg "ExecuteOption" pkg/document/operations/tree_style.go -B 2 -A 5
# Search for any WithVersionVector usage in TreeStyle
rg "WithVersionVector" pkg/document/operations/tree_style.go -B 2 -A 2
# Look at the complete TreeStyle.Execute implementation
ast-grep --pattern 'func (e *TreeStyle) Execute($_, $$$) $_$ {
$$$
}'
Length of output: 467
Script:
#!/bin/bash
# Look for any version vector usage in TreeStyle operation
rg "VersionVector" pkg/document/operations/tree_style.go -B 2 -A 5
# Check the complete TreeStyle struct definition
ast-grep --pattern 'type TreeStyle struct {
$$$
}'
Length of output: 1771
pkg/document/operations/tree_edit.go (1)
66-71: Validate Option Processing in Execute Method
The Execute method has been updated to accept opts ...Option. Verify that all options are correctly processed and that necessary default behaviors are maintained when options are absent.
Run the following script to check option handling:
✅ Verification successful
Let me gather more information about the option handling by checking the option type definition and its usage.
Let me check how the VersionVector option is used in the codebase to ensure it's properly handled.
Option Processing in Execute Method is Properly Implemented
The option processing in the Execute method is correctly implemented with proper initialization and handling. The evidence shows:
- The
ExecuteOptionstruct is well-defined with aVersionVectorfield - The option processing follows the functional options pattern with proper initialization of defaults
- The
WithVersionVectoroption is properly used in the codebase throughChange.Execute - Other operations in the same package follow the same pattern for consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the application of options in TreeEdit's Execute method.
# Inspect the Execute method in tree_edit.go.
rg 'func \(e \*TreeEdit\) Execute' -A 15
Length of output: 1213
Script:
#!/bin/bash
# Search for ExecuteOption struct definition and any Option type definitions
rg "type ExecuteOption" -A 5 -B 2
rg "type Option" pkg/document/operations/
# Search for any functions that create options
rg "func.*Option" pkg/document/operations/
Length of output: 2085
Script:
#!/bin/bash
# Search for WithVersionVector usage
rg "WithVersionVector" -B 2 -A 2
# Search for VersionVector usage in Execute methods
ast-grep --pattern 'func ($x) Execute($root, $opts) {
$$$
VersionVector
$$$
}'
Length of output: 986
pkg/document/crdt/text.go (1)
297-298:
Ensure VersionVector Is Optional
In the Style method, versionVector is introduced. Confirm that the method handles cases where versionVector is nil to prevent potential nil dereference errors.
Run the following script to check for nil handling:
✅ Verification successful
Let me check the full context of the nil handling to ensure it's properly implemented.
VersionVector is properly handled for nil case
The code already includes proper nil handling for versionVector. When versionVector is nil, it sets clientLamportAtChange to time.MaxLamport. For non-nil cases, it safely retrieves the lamport value using the Get method with proper existence checking. This implementation prevents any potential nil dereference errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for nil handling of versionVector in Style method.
# Search for nil checks in the Style method.
rg 'func \(t \*Text\) Style\(' -A 30 | rg 'if versionVector == nil'
Length of output: 123
Script:
#!/bin/bash
# Get the full context of the Style method including the nil check
rg 'func \(t \*Text\) Style\(' -A 40 pkg/document/crdt/text.go
Length of output: 1072
pkg/document/crdt/rga_tree_split.go (1)
508-509:
Handle Nil VersionVector in deleteNodes
In the deleteNodes method, ensure that a nil versionVector is handled properly to prevent runtime errors.
Run the following script to confirm nil handling:
pkg/document/crdt/tree.go (2)
804-810: Ensure Proper Handling of VersionVector
In collectBetween, versionVector is passed as a parameter. Double-check that all usages within the method handle cases where versionVector may be nil.
Run the following script to check for nil handling:
✅ Verification successful
Let me get more context about how versionVector is used within the collectBetween method.
versionVector is properly handled for nil case
The code already has proper nil handling for versionVector parameter in the collectBetween method. When versionVector is nil, it sets clientLamportAtChange = time.MaxLamport, and when it's not nil, it properly retrieves the lamport value from the vector with fallback to 0 if the actor is not found. This is a complete and safe implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for nil handling of versionVector in collectBetween method.
# Search for nil checks in the collectBetween method.
rg 'func \(t \*Tree\) collectBetween\(' -A 50 | rg 'if versionVector == nil'
Length of output: 133
Script:
#!/bin/bash
# Get the full implementation of collectBetween method to see the nil check in context
ast-grep --pattern $'func (t *Tree) collectBetween(
$$$
) ([]*TreeNode, []*TreeNode, error) {
$$$
}'
Length of output: 3911
958-970:
Handle Nil VersionVector in Style Method
When applying styles, ensure that the method properly handles a nil versionVector to prevent runtime exceptions.
Run the following script to verify nil handling:
✅ Verification successful
Let me gather more context about the Style method implementation to verify the nil handling properly.
Let me check the canStyle method to fully understand how the clientLamportAtChange is used.
Nil VersionVector is Properly Handled
The code correctly handles nil versionVector by setting clientLamportAtChange to time.MaxLamport, which effectively allows styling any node as it will always satisfy the condition n.id.CreatedAt.Lamport() <= clientLamportAtChange in the canStyle method. This is a safe default behavior that prevents runtime exceptions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify handling of nil VersionVector in Style method.
# Search for nil checks in Style method.
rg 'func \(t \*Tree\) Style\(' -A 50 | rg 'if versionVector == nil'
Length of output: 123
Script:
#!/bin/bash
# Get the full Style method implementation and surrounding context
rg 'func \(t \*Tree\) Style\(' -A 70 pkg/document/crdt/tree.go
Length of output: 1882
Script:
#!/bin/bash
# Search for canStyle method implementation
rg 'func \(.*\) canStyle\(' -A 10 pkg/document/crdt/tree.go
Length of output: 427
pkg/document/operations/operation.go (3)
34-38: LGTM: Well-structured ExecuteOption type
The ExecuteOption struct is well-designed with clear documentation and follows single responsibility principle.
40-48: LGTM: Good use of functional options pattern
The Option type and WithVersionVector helper follow Go best practices for extensible configuration. This pattern allows for future additions of execution options without breaking changes.
53-53: Verify impact of Execute interface change
The Operation interface has been updated to accept variadic options. While this maintains backward compatibility, we should verify all implementations are updated.
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax issues. Let's try another pattern:
All Execute implementations have been properly updated
The ripgrep results show that all implementations of the Execute method have been updated to include the variadic options parameter. Here's what we found:
- All operation types (TreeEdit, Style, TreeStyle, Remove, Increase, ArraySet, Edit, Move, Set, Add) have the correct signature:
Execute(root *crdt.Root, opts ...Option) error - Some implementations explicitly use the options (TreeEdit, Style, TreeStyle, Edit) by initializing an
ExecuteOption - Others acknowledge but don't use the options parameter (Remove, Increase, ArraySet, Move, Set, Add) by using
_ ...Option - The Change type in pkg/document/change/change.go has a different signature as it's not part of the operations package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of Execute method to ensure they've been updated
ast-grep --pattern 'func ($v $_) Execute(root *crdt.Root) error'
Length of output: 66
Script:
#!/bin/bash
# Let's try a different approach to find Execute implementations
# First, let's find all files that might contain Execute implementations
fd -e go | xargs rg "Execute.*\*crdt\.Root" -l
# Then let's see the actual implementations with context
fd -e go | xargs rg "Execute.*\*crdt\.Root" -B 1 -A 1
Length of output: 3769
Script:
#!/bin/bash
# Try alternative ast-grep pattern
ast-grep --pattern 'func ($v $_) Execute(root *crdt.Root, $$$) $_'
Length of output: 68
pkg/document/operations/increase.go (1)
46-46: LGTM: Proper interface implementation
The Execute method correctly implements the updated interface. The use of _ explicitly shows that options aren't needed for this operation type.
pkg/document/operations/remove.go (1)
Line range hint 51-65: Verify error handling in concurrent scenarios
The Remove operation's error handling should be reviewed in the context of concurrent operations.
✅ Verification successful
Let's analyze the test coverage and implementation details around concurrent operations to better understand the error handling in concurrent scenarios.
Let's analyze the concurrent operation handling and error cases to better understand the safety of Remove operations.
Based on the previous search results, I can now generate a final response. The code shows that Remove operations are implemented using CRDT (Conflict-free Replicated Data Type) principles, with proper error handling through DeleteByCreatedAt and RegisterRemovedElementPair. The CRDT implementation inherently handles concurrent modifications through logical timestamps (time.Ticket), and the error handling appears to be comprehensive, covering cases like ErrChildNotFound and ErrNotApplicableDataType.
The concurrent error handling in Remove operations is properly implemented
The implementation is safe for concurrent scenarios because:
- It uses CRDT-based conflict resolution with logical timestamps
- It has proper error propagation from the underlying data structures
- It maintains consistency through atomic registration of removed elements
- The operation is idempotent due to the CRDT properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering concurrent Remove operations
rg -l "TestRemove.*Concurrent" --type go
Length of output: 42
Script:
#!/bin/bash
# Let's search for test files related to Remove operations and concurrent scenarios
rg -l "TestRemove|func.*Remove.*|concurrent" --type go
# Also search for any concurrent test scenarios in the same package
fd -e go -x rg -l "func Test.*Concurrent" {} \; -p "pkg/document/operations"
# Look for any race condition or concurrent operation handling in Remove implementation
rg -A 5 "DeleteByCreatedAt|RegisterRemovedElementPair" --type go
Length of output: 11095
Script:
#!/bin/bash
# Let's look for concurrent test scenarios in the test/complex directory since we found tree_concurrency_test.go
rg -A 10 "TestTree.*Concurrent|func.*Remove.*Concurrent" test/complex/tree_concurrency_test.go
# Also check for any race condition handling or synchronization mechanisms in the CRDT implementation
rg -A 5 "sync\.|mutex|atomic" pkg/document/crdt/
# Look for error handling patterns in Remove operation tests
rg -A 5 "TestRemove.*Error|test.*Remove.*Error" --type go
Length of output: 207
pkg/document/operations/move.go (1)
55-55: Verify Operation interface implementation
The addition of the Option parameter appears to be part of a broader interface change.
✅ Verification successful
All operations consistently implement the Execute method with the same signature
The verification shows that all operation types (Move, Set, Remove, Increase, ArraySet, and Add) consistently implement the Execute method with the same signature:
Execute(root *crdt.Root, _ ...Option) error🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of Execute method across all operations
# Check if all operation types implement the same Execute signature
# Search for all Execute method implementations in operations package
ast-grep --pattern 'func (o $_) Execute(root *crdt.Root, $$$) $_'
Length of output: 6164
pkg/document/operations/add.go (1)
55-55: Verify impact on existing callers
The signature change might affect existing callers of the Execute method.
✅ Verification successful
Let me gather more information about the Execute method implementations to understand the full context.
Let me try a different pattern to find Execute method implementations.
Based on the analysis of the codebase, I can now provide the final response:
Signature change is consistent and safe
The Execute method signature change in Add operation is consistent with all other operations in the codebase. From the search results:
- All operation types (Add, Remove, Move, Set, Edit, etc.) implement Execute with the same signature:
Execute(root *crdt.Root, opts ...Option) error - The callers are already passing optional parameters in
pkg/document/change/change.goandpkg/document/document.go - The unused parameter (
_) is a common pattern across operations like Set, Move, and Remove
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts on Execute method callers
# Search for direct calls to Execute methods
rg -A 2 '\.Execute\(' --type go
Length of output: 874
Script:
#!/bin/bash
# Search for Execute method implementations in operations
ast-grep --pattern 'func $_($_ *$_) Execute($_ *crdt.Root, $$$) error {'
Length of output: 74
Script:
#!/bin/bash
# Search for Execute method implementations in operations with context
rg -U "type \w+ struct[\s\S]*?func.*Execute.*Root.*error" --type go
Length of output: 23012
pkg/document/json/text.go (1)
76-82: Verify version vector propagation in the operation context.
The operations pushed to the context should include version vector information for proper causal ordering.
Also applies to: 115-120
✅ Verification successful
Let me gather more information about how the version vector is handled in the Edit operation specifically.
Let me check one more aspect to understand how the version vector is propagated through the ticket system since the code shows ticket parameters being used.
Let me check one final aspect about how the version vector is managed in the change package since we saw it's part of the ID struct.
Based on the gathered information, I can now provide the final response:
Version vector propagation is properly implemented in the operation context
The version vector is correctly propagated through the change system:
- Version vectors are properly managed in change.ID struct
- Operations are executed with version vector context via
op.Execute(root, operations.WithVersionVector(c.ID().versionVector)) - Changes maintain causal ordering through version vectors in the ID type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if version vector is properly handled in operations
# Search for version vector usage in operations
ast-grep --pattern 'type $_ struct {
$$$
versionVector $_
$$$
}'
# Check if operations properly initialize version vector
rg -A 5 'NewEdit|NewStyle' pkg/document/operations/
Length of output: 2448
Script:
#!/bin/bash
# Check Edit operation implementation and version vector handling
ast-grep --pattern 'func NewEdit($$$) {
$$$
}'
# Check how Edit operation is executed/applied
rg -A 10 'func.*Edit.*execute' pkg/document/operations/
# Check if version vector is passed through the Text.Edit call
rg -A 5 'func.*Edit' pkg/document/json/text.go
Length of output: 255
Script:
#!/bin/bash
# Check ticket and version vector relationship
rg -A 10 "type Ticket struct" pkg/document/time/
# Check how tickets are created/handled
ast-grep --pattern 'func New.*Ticket($$$) {
$$$
}'
# Look for version vector initialization in ticket creation
rg -A 5 "NewTicket" pkg/document/time/
Length of output: 3909
Script:
#!/bin/bash
# Check how version vector is managed in changes
rg -A 10 "func.*NewID" pkg/document/change/
# Look for version vector initialization
rg -A 5 "VersionVector" pkg/document/change/
# Check if Edit operations are connected to change IDs
rg -A 5 "func.*execute.*Edit" pkg/document/
Length of output: 4475
pkg/document/operations/edit.go (2)
57-62: LGTM! Clean constructor implementation
The constructor has been simplified by removing the maxCreatedAtMapByActor parameter while maintaining all essential fields.
67-72: Verify error handling for version vector
The Execute method now correctly uses the version vector from options, but we should verify error handling when version vector is nil.
Also applies to: 77-77
✅ Verification successful
Based on the results, I can now generate the final response:
Version vector nil handling is properly implemented
The codebase already has comprehensive nil handling for version vectors across multiple components:
- In
pkg/document/crdt/text.go,tree.go, andrga_tree_split.go, nil version vectors are handled by defaulting totime.MaxLamport - The
ExecuteOptionstruct inoperations/operation.goproperly defines the version vector field - Version vector is safely passed through
operations.WithVersionVector()in the change execution flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version vector nil checks in Text.Edit implementation
ast-grep --pattern 'func (t *Text) Edit($_, $_, $_, $_, $_, versionVector $_) ($_, $_, error) {
$$$
if versionVector == nil {
$$$
}
$$$
}'
Length of output: 152
Script:
#!/bin/bash
# Let's first find the Text.Edit implementation
ast-grep --pattern 'func (t *Text) Edit($$$) {
$$$
}'
# Also search for any version vector related error handling
rg -A 5 "versionVector.*nil"
# Let's also check the ExecuteOption struct definition
ast-grep --pattern 'type ExecuteOption struct {
$$$
}'
Length of output: 4804
pkg/document/operations/style.go (2)
51-55: LGTM! Consistent constructor implementation
The constructor follows the same pattern as Edit operation, maintaining consistency across the codebase.
60-64: Verify edge cases in Style operation
The Execute method implementation looks good, but we should verify handling of edge cases:
- Empty style attributes
- Overlapping style operations
Also applies to: 72-72
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Style operation's edge cases are properly handled
The codebase shows robust handling of edge cases in the Style operation:
- The implementation includes proper error handling and type checking
- Concurrent style operations are tested thoroughly (TestTreeConcurrencyStyleStyle)
- The RGATreeSplitNode has a
canStylevalidation method that checks timing conditions - Style operations are validated against removed nodes through
canStyle
While empty attributes aren't explicitly rejected, this is acceptable as applying empty styles is a valid no-op operation that maintains consistency with the CRDT model.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for edge case handling in Text.Style implementation
ast-grep --pattern 'func (t *Text) Style($_, $_, attributes $_, $_, versionVector $_) ($_, error) {
$$$
if len(attributes) == 0 {
$$$
}
$$$
}'
# Search for overlapping style operation handling
rg -A 5 "overlapping.*style" --type go
Length of output: 197
Script:
#!/bin/bash
# Let's try a broader search for the Style implementation and its edge cases
ast-grep --pattern 'func ($_ *Style) Execute($_, $_) error {
$$$
}'
# Search for any style-related error handling
rg "return.*err" -A 3 -B 3 pkg/document/operations/style.go
# Look for style-related test cases that might reveal edge case handling
rg "func Test.*Style" -A 5 --type go
# Search for style validation or attribute checks
rg "attributes|style" -A 3 -B 3 pkg/document/operations/style.go
Length of output: 3903
Script:
#!/bin/bash
# Let's check the actual Text.Style implementation in CRDT
ast-grep --pattern 'func ($_ *Text) Style($_, $_, $_, $_, $_) ($_, error) {
$$$
}'
# Look for style-related test cases in the test directory
rg -A 10 "TestStyle|test.*style" test/ --type go
# Check for any validation of style attributes
rg -B 3 -A 3 "validate.*style|check.*style|empty.*style" --type go
Length of output: 1776
pkg/document/crdt/root_test.go (1)
68-68: LGTM! Test modifications are consistent.
The changes to text.Edit calls align with the version vector refactoring while maintaining the same test coverage and assertions.
Also applies to: 75-75, 82-82, 89-89, 128-128, 160-160, 179-179, 186-186, 193-193
api/converter/from_bytes.go (1)
337-337: Verify impact on tombstone handling.
The change from MaxTicket to MaxLamport aligns with the version vector refactoring. However, we should verify this doesn't affect tombstone handling in edge cases.
Let's check for related tombstone handling code:
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Change from MaxTicket to MaxLamport is safe for tombstone handling
The implementation shows that Remove() is consistently implemented across all CRDT types (Array, Text, Object, etc.) to use time.Ticket for tombstone tracking. The change to MaxLamport is safe because:
- All Remove() implementations check timestamps using After() comparison
- MaxLamport is used only as a sentinel value to mark permanent deletion
- The tombstone logic relies on the After() comparison which remains valid with MaxLamport
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for tombstone-related code that might be affected
rg -A 3 "Remove.*MaxLamport|Remove.*MaxTicket"
Length of output: 233
Script:
#!/bin/bash
# Let's check for any other tombstone-related code and MaxLamport/MaxTicket usage
rg -A 3 "tombstone|Tombstone"
# Also check for other Remove method implementations
ast-grep --pattern 'func $_($_, $_, $_) {
$$$
Remove($_, $_)
$$$
}'
Length of output: 76863
Script:
#!/bin/bash
# Let's check the implementation of Remove method and its usage
ast-grep --pattern 'type $_ interface {
$$$
Remove($_, $_)
$$$
}'
# Also check for any tests involving Remove and MaxLamport
rg -A 3 "Remove.*MaxLamport|Remove.*MaxTicket" test/
Length of output: 128
Script:
#!/bin/bash
# Let's check the RGATreeSplitNode implementation which likely contains the Remove method
rg -A 5 "func.*Remove" pkg/document/crdt/rga_tree_split.go
# Also check for any tests or usages of Remove with MaxLamport
rg -A 3 "Remove.*Lamport|Remove.*Ticket" pkg/document/crdt/
Length of output: 8337
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Go Benchmark
| Benchmark suite | Current: 40bf57f | Previous: 05d433a | Ratio |
|---|---|---|---|
BenchmarkDocument/constructor_test |
1509 ns/op 1337 B/op 24 allocs/op |
1474 ns/op 1337 B/op 24 allocs/op |
1.02 |
BenchmarkDocument/constructor_test - ns/op |
1509 ns/op |
1474 ns/op |
1.02 |
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 |
950.3 ns/op 1305 B/op 22 allocs/op |
1005 ns/op 1305 B/op 22 allocs/op |
0.95 |
BenchmarkDocument/status_test - ns/op |
950.3 ns/op |
1005 ns/op |
0.95 |
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 |
7734 ns/op 7529 B/op 134 allocs/op |
7831 ns/op 7553 B/op 136 allocs/op |
0.99 |
BenchmarkDocument/equals_test - ns/op |
7734 ns/op |
7831 ns/op |
0.99 |
BenchmarkDocument/equals_test - B/op |
7529 B/op |
7553 B/op |
1.00 |
BenchmarkDocument/equals_test - allocs/op |
134 allocs/op |
136 allocs/op |
0.99 |
BenchmarkDocument/nested_update_test |
19745 ns/op 12395 B/op 264 allocs/op |
19891 ns/op 12539 B/op 276 allocs/op |
0.99 |
BenchmarkDocument/nested_update_test - ns/op |
19745 ns/op |
19891 ns/op |
0.99 |
BenchmarkDocument/nested_update_test - B/op |
12395 B/op |
12539 B/op |
0.99 |
BenchmarkDocument/nested_update_test - allocs/op |
264 allocs/op |
276 allocs/op |
0.96 |
BenchmarkDocument/delete_test |
23282 ns/op 15923 B/op 347 allocs/op |
23406 ns/op 16091 B/op 361 allocs/op |
0.99 |
BenchmarkDocument/delete_test - ns/op |
23282 ns/op |
23406 ns/op |
0.99 |
BenchmarkDocument/delete_test - B/op |
15923 B/op |
16091 B/op |
0.99 |
BenchmarkDocument/delete_test - allocs/op |
347 allocs/op |
361 allocs/op |
0.96 |
BenchmarkDocument/object_test |
8754 ns/op 7073 B/op 122 allocs/op |
8901 ns/op 7121 B/op 126 allocs/op |
0.98 |
BenchmarkDocument/object_test - ns/op |
8754 ns/op |
8901 ns/op |
0.98 |
BenchmarkDocument/object_test - B/op |
7073 B/op |
7121 B/op |
0.99 |
BenchmarkDocument/object_test - allocs/op |
122 allocs/op |
126 allocs/op |
0.97 |
BenchmarkDocument/array_test |
29611 ns/op 12203 B/op 278 allocs/op |
30528 ns/op 12371 B/op 292 allocs/op |
0.97 |
BenchmarkDocument/array_test - ns/op |
29611 ns/op |
30528 ns/op |
0.97 |
BenchmarkDocument/array_test - B/op |
12203 B/op |
12371 B/op |
0.99 |
BenchmarkDocument/array_test - allocs/op |
278 allocs/op |
292 allocs/op |
0.95 |
BenchmarkDocument/text_test |
32208 ns/op 15323 B/op 492 allocs/op |
32697 ns/op 14803 B/op 494 allocs/op |
0.99 |
BenchmarkDocument/text_test - ns/op |
32208 ns/op |
32697 ns/op |
0.99 |
BenchmarkDocument/text_test - B/op |
15323 B/op |
14803 B/op |
1.04 |
BenchmarkDocument/text_test - allocs/op |
492 allocs/op |
494 allocs/op |
1.00 |
BenchmarkDocument/text_composition_test |
31080 ns/op 18718 B/op 504 allocs/op |
29857 ns/op 16694 B/op 504 allocs/op |
1.04 |
BenchmarkDocument/text_composition_test - ns/op |
31080 ns/op |
29857 ns/op |
1.04 |
BenchmarkDocument/text_composition_test - B/op |
18718 B/op |
16694 B/op |
1.12 |
BenchmarkDocument/text_composition_test - allocs/op |
504 allocs/op |
504 allocs/op |
1 |
BenchmarkDocument/rich_text_test |
83687 ns/op 40180 B/op 1183 allocs/op |
84689 ns/op 38156 B/op 1183 allocs/op |
0.99 |
BenchmarkDocument/rich_text_test - ns/op |
83687 ns/op |
84689 ns/op |
0.99 |
BenchmarkDocument/rich_text_test - B/op |
40180 B/op |
38156 B/op |
1.05 |
BenchmarkDocument/rich_text_test - allocs/op |
1183 allocs/op |
1183 allocs/op |
1 |
BenchmarkDocument/counter_test |
18659 ns/op 11874 B/op 258 allocs/op |
19383 ns/op 12258 B/op 290 allocs/op |
0.96 |
BenchmarkDocument/counter_test - ns/op |
18659 ns/op |
19383 ns/op |
0.96 |
BenchmarkDocument/counter_test - B/op |
11874 B/op |
12258 B/op |
0.97 |
BenchmarkDocument/counter_test - allocs/op |
258 allocs/op |
290 allocs/op |
0.89 |
BenchmarkDocument/text_edit_gc_100 |
1338510 ns/op 872588 B/op 17282 allocs/op |
1307957 ns/op 818182 B/op 17283 allocs/op |
1.02 |
BenchmarkDocument/text_edit_gc_100 - ns/op |
1338510 ns/op |
1307957 ns/op |
1.02 |
BenchmarkDocument/text_edit_gc_100 - B/op |
872588 B/op |
818182 B/op |
1.07 |
BenchmarkDocument/text_edit_gc_100 - allocs/op |
17282 allocs/op |
17283 allocs/op |
1.00 |
BenchmarkDocument/text_edit_gc_1000 |
52617456 ns/op 50546402 B/op 186741 allocs/op |
48939526 ns/op 50002789 B/op 186738 allocs/op |
1.08 |
BenchmarkDocument/text_edit_gc_1000 - ns/op |
52617456 ns/op |
48939526 ns/op |
1.08 |
BenchmarkDocument/text_edit_gc_1000 - B/op |
50546402 B/op |
50002789 B/op |
1.01 |
BenchmarkDocument/text_edit_gc_1000 - allocs/op |
186741 allocs/op |
186738 allocs/op |
1.00 |
BenchmarkDocument/text_split_gc_100 |
1953887 ns/op 1589086 B/op 15951 allocs/op |
1925624 ns/op 1541057 B/op 15854 allocs/op |
1.01 |
BenchmarkDocument/text_split_gc_100 - ns/op |
1953887 ns/op |
1925624 ns/op |
1.01 |
BenchmarkDocument/text_split_gc_100 - B/op |
1589086 B/op |
1541057 B/op |
1.03 |
BenchmarkDocument/text_split_gc_100 - allocs/op |
15951 allocs/op |
15854 allocs/op |
1.01 |
BenchmarkDocument/text_split_gc_1000 |
119246837 ns/op 141483353 B/op 186149 allocs/op |
114747096 ns/op 141002897 B/op 185152 allocs/op |
1.04 |
BenchmarkDocument/text_split_gc_1000 - ns/op |
119246837 ns/op |
114747096 ns/op |
1.04 |
BenchmarkDocument/text_split_gc_1000 - B/op |
141483353 B/op |
141002897 B/op |
1.00 |
BenchmarkDocument/text_split_gc_1000 - allocs/op |
186149 allocs/op |
185152 allocs/op |
1.01 |
BenchmarkDocument/text_delete_all_10000 |
15635814 ns/op 10212945 B/op 55685 allocs/op |
15438574 ns/op 10213220 B/op 55685 allocs/op |
1.01 |
BenchmarkDocument/text_delete_all_10000 - ns/op |
15635814 ns/op |
15438574 ns/op |
1.01 |
BenchmarkDocument/text_delete_all_10000 - B/op |
10212945 B/op |
10213220 B/op |
1.00 |
BenchmarkDocument/text_delete_all_10000 - allocs/op |
55685 allocs/op |
55685 allocs/op |
1 |
BenchmarkDocument/text_delete_all_100000 |
298647266 ns/op 142991912 B/op 561729 allocs/op |
275590142 ns/op 142997668 B/op 561795 allocs/op |
1.08 |
BenchmarkDocument/text_delete_all_100000 - ns/op |
298647266 ns/op |
275590142 ns/op |
1.08 |
BenchmarkDocument/text_delete_all_100000 - B/op |
142991912 B/op |
142997668 B/op |
1.00 |
BenchmarkDocument/text_delete_all_100000 - allocs/op |
561729 allocs/op |
561795 allocs/op |
1.00 |
BenchmarkDocument/text_100 |
221475 ns/op 120491 B/op 5182 allocs/op |
233603 ns/op 114115 B/op 5284 allocs/op |
0.95 |
BenchmarkDocument/text_100 - ns/op |
221475 ns/op |
233603 ns/op |
0.95 |
BenchmarkDocument/text_100 - B/op |
120491 B/op |
114115 B/op |
1.06 |
BenchmarkDocument/text_100 - allocs/op |
5182 allocs/op |
5284 allocs/op |
0.98 |
BenchmarkDocument/text_1000 |
2418287 ns/op 1171277 B/op 51086 allocs/op |
2510023 ns/op 1107302 B/op 52088 allocs/op |
0.96 |
BenchmarkDocument/text_1000 - ns/op |
2418287 ns/op |
2510023 ns/op |
0.96 |
BenchmarkDocument/text_1000 - B/op |
1171277 B/op |
1107302 B/op |
1.06 |
BenchmarkDocument/text_1000 - allocs/op |
51086 allocs/op |
52088 allocs/op |
0.98 |
BenchmarkDocument/array_1000 |
1212404 ns/op 1091648 B/op 11833 allocs/op |
1342442 ns/op 1115605 B/op 13835 allocs/op |
0.90 |
BenchmarkDocument/array_1000 - ns/op |
1212404 ns/op |
1342442 ns/op |
0.90 |
BenchmarkDocument/array_1000 - B/op |
1091648 B/op |
1115605 B/op |
0.98 |
BenchmarkDocument/array_1000 - allocs/op |
11833 allocs/op |
13835 allocs/op |
0.86 |
BenchmarkDocument/array_10000 |
13269169 ns/op 9798804 B/op 120293 allocs/op |
14075506 ns/op 10039735 B/op 140297 allocs/op |
0.94 |
BenchmarkDocument/array_10000 - ns/op |
13269169 ns/op |
14075506 ns/op |
0.94 |
BenchmarkDocument/array_10000 - B/op |
9798804 B/op |
10039735 B/op |
0.98 |
BenchmarkDocument/array_10000 - allocs/op |
120293 allocs/op |
140297 allocs/op |
0.86 |
BenchmarkDocument/array_gc_100 |
146373 ns/op 133284 B/op 1266 allocs/op |
161447 ns/op 135726 B/op 1470 allocs/op |
0.91 |
BenchmarkDocument/array_gc_100 - ns/op |
146373 ns/op |
161447 ns/op |
0.91 |
BenchmarkDocument/array_gc_100 - B/op |
133284 B/op |
135726 B/op |
0.98 |
BenchmarkDocument/array_gc_100 - allocs/op |
1266 allocs/op |
1470 allocs/op |
0.86 |
BenchmarkDocument/array_gc_1000 |
1380157 ns/op 1159707 B/op 12883 allocs/op |
1525383 ns/op 1183892 B/op 14887 allocs/op |
0.90 |
BenchmarkDocument/array_gc_1000 - ns/op |
1380157 ns/op |
1525383 ns/op |
0.90 |
BenchmarkDocument/array_gc_1000 - B/op |
1159707 B/op |
1183892 B/op |
0.98 |
BenchmarkDocument/array_gc_1000 - allocs/op |
12883 allocs/op |
14887 allocs/op |
0.87 |
BenchmarkDocument/counter_1000 |
199991 ns/op 193337 B/op 5773 allocs/op |
268294 ns/op 217361 B/op 7775 allocs/op |
0.75 |
BenchmarkDocument/counter_1000 - ns/op |
199991 ns/op |
268294 ns/op |
0.75 |
BenchmarkDocument/counter_1000 - B/op |
193337 B/op |
217361 B/op |
0.89 |
BenchmarkDocument/counter_1000 - allocs/op |
5773 allocs/op |
7775 allocs/op |
0.74 |
BenchmarkDocument/counter_10000 |
2191032 ns/op 2088253 B/op 59780 allocs/op |
2780585 ns/op 2328286 B/op 79782 allocs/op |
0.79 |
BenchmarkDocument/counter_10000 - ns/op |
2191032 ns/op |
2780585 ns/op |
0.79 |
BenchmarkDocument/counter_10000 - B/op |
2088253 B/op |
2328286 B/op |
0.90 |
BenchmarkDocument/counter_10000 - allocs/op |
59780 allocs/op |
79782 allocs/op |
0.75 |
BenchmarkDocument/object_1000 |
1377517 ns/op 1428469 B/op 9851 allocs/op |
1512098 ns/op 1452381 B/op 11851 allocs/op |
0.91 |
BenchmarkDocument/object_1000 - ns/op |
1377517 ns/op |
1512098 ns/op |
0.91 |
BenchmarkDocument/object_1000 - B/op |
1428469 B/op |
1452381 B/op |
0.98 |
BenchmarkDocument/object_1000 - allocs/op |
9851 allocs/op |
11851 allocs/op |
0.83 |
BenchmarkDocument/object_10000 |
15131536 ns/op 12167416 B/op 100568 allocs/op |
15898076 ns/op 12406718 B/op 120565 allocs/op |
0.95 |
BenchmarkDocument/object_10000 - ns/op |
15131536 ns/op |
15898076 ns/op |
0.95 |
BenchmarkDocument/object_10000 - B/op |
12167416 B/op |
12406718 B/op |
0.98 |
BenchmarkDocument/object_10000 - allocs/op |
100568 allocs/op |
120565 allocs/op |
0.83 |
BenchmarkDocument/tree_100 |
1007181 ns/op 943956 B/op 6103 allocs/op |
1063348 ns/op 935983 B/op 6205 allocs/op |
0.95 |
BenchmarkDocument/tree_100 - ns/op |
1007181 ns/op |
1063348 ns/op |
0.95 |
BenchmarkDocument/tree_100 - B/op |
943956 B/op |
935983 B/op |
1.01 |
BenchmarkDocument/tree_100 - allocs/op |
6103 allocs/op |
6205 allocs/op |
0.98 |
BenchmarkDocument/tree_1000 |
72006220 ns/op 86460428 B/op 60116 allocs/op |
76580627 ns/op 86380589 B/op 61118 allocs/op |
0.94 |
BenchmarkDocument/tree_1000 - ns/op |
72006220 ns/op |
76580627 ns/op |
0.94 |
BenchmarkDocument/tree_1000 - B/op |
86460428 B/op |
86380589 B/op |
1.00 |
BenchmarkDocument/tree_1000 - allocs/op |
60116 allocs/op |
61118 allocs/op |
0.98 |
BenchmarkDocument/tree_10000 |
9099559002 ns/op 8580673792 B/op 600247 allocs/op |
9549194510 ns/op 8579862488 B/op 610235 allocs/op |
0.95 |
BenchmarkDocument/tree_10000 - ns/op |
9099559002 ns/op |
9549194510 ns/op |
0.95 |
BenchmarkDocument/tree_10000 - B/op |
8580673792 B/op |
8579862488 B/op |
1.00 |
BenchmarkDocument/tree_10000 - allocs/op |
600247 allocs/op |
610235 allocs/op |
0.98 |
BenchmarkDocument/tree_delete_all_1000 |
73337237 ns/op 87531666 B/op 75271 allocs/op |
80156802 ns/op 87428977 B/op 76271 allocs/op |
0.91 |
BenchmarkDocument/tree_delete_all_1000 - ns/op |
73337237 ns/op |
80156802 ns/op |
0.91 |
BenchmarkDocument/tree_delete_all_1000 - B/op |
87531666 B/op |
87428977 B/op |
1.00 |
BenchmarkDocument/tree_delete_all_1000 - allocs/op |
75271 allocs/op |
76271 allocs/op |
0.99 |
BenchmarkDocument/tree_edit_gc_100 |
3783317 ns/op 4147153 B/op 15146 allocs/op |
3964356 ns/op 4089786 B/op 15149 allocs/op |
0.95 |
BenchmarkDocument/tree_edit_gc_100 - ns/op |
3783317 ns/op |
3964356 ns/op |
0.95 |
BenchmarkDocument/tree_edit_gc_100 - B/op |
4147153 B/op |
4089786 B/op |
1.01 |
BenchmarkDocument/tree_edit_gc_100 - allocs/op |
15146 allocs/op |
15149 allocs/op |
1.00 |
BenchmarkDocument/tree_edit_gc_1000 |
292252817 ns/op 383745410 B/op 154847 allocs/op |
314241872 ns/op 383175216 B/op 154888 allocs/op |
0.93 |
BenchmarkDocument/tree_edit_gc_1000 - ns/op |
292252817 ns/op |
314241872 ns/op |
0.93 |
BenchmarkDocument/tree_edit_gc_1000 - B/op |
383745410 B/op |
383175216 B/op |
1.00 |
BenchmarkDocument/tree_edit_gc_1000 - allocs/op |
154847 allocs/op |
154888 allocs/op |
1.00 |
BenchmarkDocument/tree_split_gc_100 |
2487798 ns/op 2413070 B/op 11131 allocs/op |
2638739 ns/op 2363462 B/op 11034 allocs/op |
0.94 |
BenchmarkDocument/tree_split_gc_100 - ns/op |
2487798 ns/op |
2638739 ns/op |
0.94 |
BenchmarkDocument/tree_split_gc_100 - B/op |
2413070 B/op |
2363462 B/op |
1.02 |
BenchmarkDocument/tree_split_gc_100 - allocs/op |
11131 allocs/op |
11034 allocs/op |
1.01 |
BenchmarkDocument/tree_split_gc_1000 |
179561248 ns/op 222253821 B/op 122002 allocs/op |
191415530 ns/op 221758888 B/op 121017 allocs/op |
0.94 |
BenchmarkDocument/tree_split_gc_1000 - ns/op |
179561248 ns/op |
191415530 ns/op |
0.94 |
BenchmarkDocument/tree_split_gc_1000 - B/op |
222253821 B/op |
221758888 B/op |
1.00 |
BenchmarkDocument/tree_split_gc_1000 - allocs/op |
122002 allocs/op |
121017 allocs/op |
1.01 |
BenchmarkRPC/client_to_server |
416202854 ns/op 19411320 B/op 223195 allocs/op |
406545919 ns/op 19123061 B/op 203826 allocs/op |
1.02 |
BenchmarkRPC/client_to_server - ns/op |
416202854 ns/op |
406545919 ns/op |
1.02 |
BenchmarkRPC/client_to_server - B/op |
19411320 B/op |
19123061 B/op |
1.02 |
BenchmarkRPC/client_to_server - allocs/op |
223195 allocs/op |
203826 allocs/op |
1.10 |
BenchmarkRPC/client_to_client_via_server |
765785848 ns/op 49680156 B/op 474764 allocs/op |
748154347 ns/op 44038776 B/op 425152 allocs/op |
1.02 |
BenchmarkRPC/client_to_client_via_server - ns/op |
765785848 ns/op |
748154347 ns/op |
1.02 |
BenchmarkRPC/client_to_client_via_server - B/op |
49680156 B/op |
44038776 B/op |
1.13 |
BenchmarkRPC/client_to_client_via_server - allocs/op |
474764 allocs/op |
425152 allocs/op |
1.12 |
BenchmarkRPC/attach_large_document |
1252718455 ns/op 1920962192 B/op 11980 allocs/op |
1306745860 ns/op 1911246880 B/op 11942 allocs/op |
0.96 |
BenchmarkRPC/attach_large_document - ns/op |
1252718455 ns/op |
1306745860 ns/op |
0.96 |
BenchmarkRPC/attach_large_document - B/op |
1920962192 B/op |
1911246880 B/op |
1.01 |
BenchmarkRPC/attach_large_document - allocs/op |
11980 allocs/op |
11942 allocs/op |
1.00 |
BenchmarkRPC/adminCli_to_server |
525836075 ns/op 37211340 B/op 289710 allocs/op |
521886449 ns/op 35993768 B/op 289700 allocs/op |
1.01 |
BenchmarkRPC/adminCli_to_server - ns/op |
525836075 ns/op |
521886449 ns/op |
1.01 |
BenchmarkRPC/adminCli_to_server - B/op |
37211340 B/op |
35993768 B/op |
1.03 |
BenchmarkRPC/adminCli_to_server - allocs/op |
289710 allocs/op |
289700 allocs/op |
1.00 |
BenchmarkLocker |
66.6 ns/op 16 B/op 1 allocs/op |
66.5 ns/op 16 B/op 1 allocs/op |
1.00 |
BenchmarkLocker - ns/op |
66.6 ns/op |
66.5 ns/op |
1.00 |
BenchmarkLocker - B/op |
16 B/op |
16 B/op |
1 |
BenchmarkLocker - allocs/op |
1 allocs/op |
1 allocs/op |
1 |
BenchmarkLockerParallel |
42.86 ns/op 0 B/op 0 allocs/op |
39.41 ns/op 0 B/op 0 allocs/op |
1.09 |
BenchmarkLockerParallel - ns/op |
42.86 ns/op |
39.41 ns/op |
1.09 |
BenchmarkLockerParallel - B/op |
0 B/op |
0 B/op |
1 |
BenchmarkLockerParallel - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkLockerMoreKeys |
147.8 ns/op 15 B/op 0 allocs/op |
157 ns/op 15 B/op 0 allocs/op |
0.94 |
BenchmarkLockerMoreKeys - ns/op |
147.8 ns/op |
157 ns/op |
0.94 |
BenchmarkLockerMoreKeys - B/op |
15 B/op |
15 B/op |
1 |
BenchmarkLockerMoreKeys - allocs/op |
0 allocs/op |
0 allocs/op |
1 |
BenchmarkChange/Push_10_Changes |
4441167 ns/op 149770 B/op 1576 allocs/op |
4420583 ns/op 150135 B/op 1580 allocs/op |
1.00 |
BenchmarkChange/Push_10_Changes - ns/op |
4441167 ns/op |
4420583 ns/op |
1.00 |
BenchmarkChange/Push_10_Changes - B/op |
149770 B/op |
150135 B/op |
1.00 |
BenchmarkChange/Push_10_Changes - allocs/op |
1576 allocs/op |
1580 allocs/op |
1.00 |
BenchmarkChange/Push_100_Changes |
16297516 ns/op 780390 B/op 8283 allocs/op |
15648865 ns/op 796699 B/op 8290 allocs/op |
1.04 |
BenchmarkChange/Push_100_Changes - ns/op |
16297516 ns/op |
15648865 ns/op |
1.04 |
BenchmarkChange/Push_100_Changes - B/op |
780390 B/op |
796699 B/op |
0.98 |
BenchmarkChange/Push_100_Changes - allocs/op |
8283 allocs/op |
8290 allocs/op |
1.00 |
BenchmarkChange/Push_1000_Changes |
131562542 ns/op 7141528 B/op 77294 allocs/op |
125330528 ns/op 7068260 B/op 77298 allocs/op |
1.05 |
BenchmarkChange/Push_1000_Changes - ns/op |
131562542 ns/op |
125330528 ns/op |
1.05 |
BenchmarkChange/Push_1000_Changes - B/op |
7141528 B/op |
7068260 B/op |
1.01 |
BenchmarkChange/Push_1000_Changes - allocs/op |
77294 allocs/op |
77298 allocs/op |
1.00 |
BenchmarkChange/Pull_10_Changes |
3651756 ns/op 123770 B/op 1407 allocs/op |
3597654 ns/op 124245 B/op 1414 allocs/op |
1.02 |
BenchmarkChange/Pull_10_Changes - ns/op |
3651756 ns/op |
3597654 ns/op |
1.02 |
BenchmarkChange/Pull_10_Changes - B/op |
123770 B/op |
124245 B/op |
1.00 |
BenchmarkChange/Pull_10_Changes - allocs/op |
1407 allocs/op |
1414 allocs/op |
1.00 |
BenchmarkChange/Pull_100_Changes |
5180411 ns/op 352371 B/op 5042 allocs/op |
5170871 ns/op 353051 B/op 5048 allocs/op |
1.00 |
BenchmarkChange/Pull_100_Changes - ns/op |
5180411 ns/op |
5170871 ns/op |
1.00 |
BenchmarkChange/Pull_100_Changes - B/op |
352371 B/op |
353051 B/op |
1.00 |
BenchmarkChange/Pull_100_Changes - allocs/op |
5042 allocs/op |
5048 allocs/op |
1.00 |
BenchmarkChange/Pull_1000_Changes |
10538666 ns/op 2229544 B/op 43656 allocs/op |
10471280 ns/op 2227521 B/op 43663 allocs/op |
1.01 |
BenchmarkChange/Pull_1000_Changes - ns/op |
10538666 ns/op |
10471280 ns/op |
1.01 |
BenchmarkChange/Pull_1000_Changes - B/op |
2229544 B/op |
2227521 B/op |
1.00 |
BenchmarkChange/Pull_1000_Changes - allocs/op |
43656 allocs/op |
43663 allocs/op |
1.00 |
BenchmarkSnapshot/Push_3KB_snapshot |
18654422 ns/op 920392 B/op 8288 allocs/op |
18258155 ns/op 909749 B/op 8290 allocs/op |
1.02 |
BenchmarkSnapshot/Push_3KB_snapshot - ns/op |
18654422 ns/op |
18258155 ns/op |
1.02 |
BenchmarkSnapshot/Push_3KB_snapshot - B/op |
920392 B/op |
909749 B/op |
1.01 |
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op |
8288 allocs/op |
8290 allocs/op |
1.00 |
BenchmarkSnapshot/Push_30KB_snapshot |
135673981 ns/op 7914073 B/op 82224 allocs/op |
129975892 ns/op 7629875 B/op 79592 allocs/op |
1.04 |
BenchmarkSnapshot/Push_30KB_snapshot - ns/op |
135673981 ns/op |
129975892 ns/op |
1.04 |
BenchmarkSnapshot/Push_30KB_snapshot - B/op |
7914073 B/op |
7629875 B/op |
1.04 |
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op |
82224 allocs/op |
79592 allocs/op |
1.03 |
BenchmarkSnapshot/Pull_3KB_snapshot |
7394914 ns/op 1141790 B/op 19609 allocs/op |
7593955 ns/op 1147862 B/op 20022 allocs/op |
0.97 |
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op |
7394914 ns/op |
7593955 ns/op |
0.97 |
BenchmarkSnapshot/Pull_3KB_snapshot - B/op |
1141790 B/op |
1147862 B/op |
0.99 |
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op |
19609 allocs/op |
20022 allocs/op |
0.98 |
BenchmarkSnapshot/Pull_30KB_snapshot |
19282617 ns/op 9343614 B/op 189559 allocs/op |
19425684 ns/op 9384420 B/op 193530 allocs/op |
0.99 |
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op |
19282617 ns/op |
19425684 ns/op |
0.99 |
BenchmarkSnapshot/Pull_30KB_snapshot - B/op |
9343614 B/op |
9384420 B/op |
1.00 |
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op |
189559 allocs/op |
193530 allocs/op |
0.98 |
BenchmarkSplayTree/stress_test_100000 |
0.1901 ns/op 0 B/op 0 allocs/op |
0.1926 ns/op 0 B/op 0 allocs/op |
0.99 |
BenchmarkSplayTree/stress_test_100000 - ns/op |
0.1901 ns/op |
0.1926 ns/op |
0.99 |
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.3915 ns/op 0 B/op 0 allocs/op |
0.3935 ns/op 0 B/op 0 allocs/op |
0.99 |
BenchmarkSplayTree/stress_test_200000 - ns/op |
0.3915 ns/op |
0.3935 ns/op |
0.99 |
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.5954 ns/op 0 B/op 0 allocs/op |
0.5862 ns/op 0 B/op 0 allocs/op |
1.02 |
BenchmarkSplayTree/stress_test_300000 - ns/op |
0.5954 ns/op |
0.5862 ns/op |
1.02 |
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.01264 ns/op 0 B/op 0 allocs/op |
0.01311 ns/op 0 B/op 0 allocs/op |
0.96 |
BenchmarkSplayTree/random_access_100000 - ns/op |
0.01264 ns/op |
0.01311 ns/op |
0.96 |
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.03301 ns/op 0 B/op 0 allocs/op |
0.0323 ns/op 0 B/op 0 allocs/op |
1.02 |
BenchmarkSplayTree/random_access_200000 - ns/op |
0.03301 ns/op |
0.0323 ns/op |
1.02 |
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.04443 ns/op 0 B/op 0 allocs/op |
0.04592 ns/op 0 B/op 0 allocs/op |
0.97 |
BenchmarkSplayTree/random_access_300000 - ns/op |
0.04443 ns/op |
0.04592 ns/op |
0.97 |
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.00167 ns/op 0 B/op 0 allocs/op |
0.001989 ns/op 0 B/op 0 allocs/op |
0.84 |
BenchmarkSplayTree/editing_trace_bench - ns/op |
0.00167 ns/op |
0.001989 ns/op |
0.84 |
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 |
8231 ns/op 3765 B/op 69 allocs/op |
8950 ns/op 3765 B/op 69 allocs/op |
0.92 |
BenchmarkSync/memory_sync_10_test - ns/op |
8231 ns/op |
8950 ns/op |
0.92 |
BenchmarkSync/memory_sync_10_test - B/op |
3765 B/op |
3765 B/op |
1 |
BenchmarkSync/memory_sync_10_test - allocs/op |
69 allocs/op |
69 allocs/op |
1 |
BenchmarkSync/memory_sync_100_test |
54048 ns/op 11123 B/op 304 allocs/op |
55157 ns/op 11117 B/op 303 allocs/op |
0.98 |
BenchmarkSync/memory_sync_100_test - ns/op |
54048 ns/op |
55157 ns/op |
0.98 |
BenchmarkSync/memory_sync_100_test - B/op |
11123 B/op |
11117 B/op |
1.00 |
BenchmarkSync/memory_sync_100_test - allocs/op |
304 allocs/op |
303 allocs/op |
1.00 |
BenchmarkSync/memory_sync_1000_test |
591593 ns/op 76981 B/op 2148 allocs/op |
589024 ns/op 76964 B/op 2148 allocs/op |
1.00 |
BenchmarkSync/memory_sync_1000_test - ns/op |
591593 ns/op |
589024 ns/op |
1.00 |
BenchmarkSync/memory_sync_1000_test - B/op |
76981 B/op |
76964 B/op |
1.00 |
BenchmarkSync/memory_sync_1000_test - allocs/op |
2148 allocs/op |
2148 allocs/op |
1 |
BenchmarkSync/memory_sync_10000_test |
7254648 ns/op 750433 B/op 20451 allocs/op |
7670611 ns/op 755218 B/op 20429 allocs/op |
0.95 |
BenchmarkSync/memory_sync_10000_test - ns/op |
7254648 ns/op |
7670611 ns/op |
0.95 |
BenchmarkSync/memory_sync_10000_test - B/op |
750433 B/op |
755218 B/op |
0.99 |
BenchmarkSync/memory_sync_10000_test - allocs/op |
20451 allocs/op |
20429 allocs/op |
1.00 |
BenchmarkTextEditing |
5060030389 ns/op 3982564512 B/op 20647621 allocs/op |
4916053917 ns/op 3933723648 B/op 20752501 allocs/op |
1.03 |
BenchmarkTextEditing - ns/op |
5060030389 ns/op |
4916053917 ns/op |
1.03 |
BenchmarkTextEditing - B/op |
3982564512 B/op |
3933723648 B/op |
1.01 |
BenchmarkTextEditing - allocs/op |
20647621 allocs/op |
20752501 allocs/op |
0.99 |
BenchmarkTree/10000_vertices_to_protobuf |
4243705 ns/op 6263009 B/op 70025 allocs/op |
4061038 ns/op 6263013 B/op 70025 allocs/op |
1.04 |
BenchmarkTree/10000_vertices_to_protobuf - ns/op |
4243705 ns/op |
4061038 ns/op |
1.04 |
BenchmarkTree/10000_vertices_to_protobuf - B/op |
6263009 B/op |
6263013 B/op |
1.00 |
BenchmarkTree/10000_vertices_to_protobuf - allocs/op |
70025 allocs/op |
70025 allocs/op |
1 |
BenchmarkTree/10000_vertices_from_protobuf |
218167097 ns/op 442170276 B/op 290038 allocs/op |
219537771 ns/op 442170328 B/op 290039 allocs/op |
0.99 |
BenchmarkTree/10000_vertices_from_protobuf - ns/op |
218167097 ns/op |
219537771 ns/op |
0.99 |
BenchmarkTree/10000_vertices_from_protobuf - B/op |
442170276 B/op |
442170328 B/op |
1.00 |
BenchmarkTree/10000_vertices_from_protobuf - allocs/op |
290038 allocs/op |
290039 allocs/op |
1.00 |
BenchmarkTree/20000_vertices_to_protobuf |
8926074 ns/op 12721781 B/op 140028 allocs/op |
8550277 ns/op 12717006 B/op 140028 allocs/op |
1.04 |
BenchmarkTree/20000_vertices_to_protobuf - ns/op |
8926074 ns/op |
8550277 ns/op |
1.04 |
BenchmarkTree/20000_vertices_to_protobuf - B/op |
12721781 B/op |
12717006 B/op |
1.00 |
BenchmarkTree/20000_vertices_to_protobuf - allocs/op |
140028 allocs/op |
140028 allocs/op |
1 |
BenchmarkTree/20000_vertices_from_protobuf |
850931984 ns/op 1697271952 B/op 580044 allocs/op |
858743498 ns/op 1697272272 B/op 580045 allocs/op |
0.99 |
BenchmarkTree/20000_vertices_from_protobuf - ns/op |
850931984 ns/op |
858743498 ns/op |
0.99 |
BenchmarkTree/20000_vertices_from_protobuf - B/op |
1697271952 B/op |
1697272272 B/op |
1.00 |
BenchmarkTree/20000_vertices_from_protobuf - allocs/op |
580044 allocs/op |
580045 allocs/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf |
13831644 ns/op 19318358 B/op 210031 allocs/op |
13797475 ns/op 19325053 B/op 210030 allocs/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf - ns/op |
13831644 ns/op |
13797475 ns/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf - B/op |
19318358 B/op |
19325053 B/op |
1.00 |
BenchmarkTree/30000_vertices_to_protobuf - allocs/op |
210031 allocs/op |
210030 allocs/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf |
1955773969 ns/op 3752052200 B/op 870049 allocs/op |
1920874214 ns/op 3752052840 B/op 870136 allocs/op |
1.02 |
BenchmarkTree/30000_vertices_from_protobuf - ns/op |
1955773969 ns/op |
1920874214 ns/op |
1.02 |
BenchmarkTree/30000_vertices_from_protobuf - B/op |
3752052200 B/op |
3752052840 B/op |
1.00 |
BenchmarkTree/30000_vertices_from_protobuf - allocs/op |
870049 allocs/op |
870136 allocs/op |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/integration/gc_test.go (2)
1196-1284: Test case is comprehensive but needs minor improvementsThe test case thoroughly verifies the garbage collection behavior during document lifecycle, but has a few areas for improvement:
- The comment on line 1255 says "insert c" but the actual operation inserts "1"
- Consider adding more descriptive comments explaining the expected state after each operation
Apply this diff to fix the comment:
- }, "insert c") + }, "insert 1")
1286-1334: Track implementation of node deletion functionalityThis test case is currently skipped waiting for node deletion implementation. It would be helpful to track this as a separate task.
Would you like me to help create a GitHub issue to track the implementation of node deletion functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/document/change/change.go(1 hunks)pkg/document/document.go(2 hunks)pkg/document/internal_document.go(1 hunks)test/integration/gc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/document/change/change.go
🔇 Additional comments (6)
test/integration/gc_test.go (2)
Line range hint 32-48: Helper functions are well-designed and properly implemented
The checkVV and versionOf helper functions provide clear and reusable functionality for version vector operations throughout the tests.
Line range hint 1-31: Test file structure follows best practices
The test file demonstrates good practices:
- Well-organized test cases
- Consistent error handling
- Clear setup and teardown patterns
- Comprehensive coverage of various scenarios
pkg/document/internal_document.go (1)
329-329: LGTM: Operation source differentiation enhances concurrency control
The addition of "remote" parameter to Execute helps distinguish between local and remote operations, which is crucial for proper handling of causal-concurrent operations in a distributed system.
pkg/document/document.go (3)
173-173: LGTM: Proper identification of local operations
The addition of "local" parameter correctly identifies operations originating from user updates, maintaining proper causality tracking in the CRDT implementation.
248-248: LGTM: Consistent handling of remote operations
The addition of "remote" parameter maintains consistency with the internal document implementation and ensures proper handling of operations from other clients.
173-173: Verify complete coverage of operation source specification
The changes look good, but let's verify that all execution paths properly specify the operation source ("local" or "remote") to ensure consistent behavior.
Also applies to: 248-248, 329-329
✅ Verification successful
Based on the results, I can now generate the final response:
Operation source specification is properly implemented
All Execute calls in the document package consistently specify the operation source as either "local" or "remote":
document.go: Uses "local" for local operations and "remote" for remote operationsinternal_document.go: Uses "remote" for remote operations
The implementation in change.go correctly handles both sources by checking opSource == "remote" and applying the appropriate execution logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any Execute calls that might be missing the operation source parameter
# Search for Execute method calls
rg -A 2 'Execute\(' --type go
# Search for the Execute method definition to verify its signature
ast-grep --pattern 'Execute($_, $_, $_) {
$$$
}'
Length of output: 3855
chacha912
force-pushed
the
replace-maxcreatedat
branch
from
98f8d3b to
28d8040
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/integration/gc_test.go (2)
1196-1196: Consider a more specific test name.The current test name "attach > pushpull > detach lifecycle version vector test" could be more specific about what it's testing. Consider renaming to clarify that it specifically tests garbage collection behavior during the document lifecycle.
-t.Run("attach > pushpull > detach lifecycle version vector test (run gc at last client detaches document)", func(t *testing.T) { +t.Run("garbage collection: should clean up when last client detaches from document", func(t *testing.T) {
1258-1258: Fix incorrect comment in test case.The comment "insert c" doesn't match the actual operation which inserts "1".
- }, "insert c") + }, "insert 1")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/document/change/change.go(1 hunks)pkg/document/document.go(2 hunks)pkg/document/internal_document.go(1 hunks)test/integration/gc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/document/internal_document.go
- pkg/document/change/change.go
- pkg/document/document.go
🔇 Additional comments (1)
test/integration/gc_test.go (1)
1196-1288: LGTM! Comprehensive test coverage for garbage collection lifecycle.
The test case thoroughly verifies garbage collection behavior during document lifecycle:
- Correctly tests version vector updates during attach/detach
- Verifies garbage collection timing
- Includes proper cleanup in defer statements
- Has detailed assertions for version vectors
chacha912
force-pushed
the
replace-maxcreatedat
branch
from
28d8040 to
683bfec
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
pkg/document/operations/tree_edit.go (3)
Line range hint
93-125: Critical: Remove redundant maxCreatedAtMapByActor parameterThe
Editmethod call is still usingmaxCreatedAtMapByActorwhile also passingoptions.VersionVector. This is inconsistent with the transition to version vectors.Update the method call to remove the redundant parameter:
_, pairs, err := obj.Edit( e.from, e.to, contents, e.splitLevel, e.executedAt, func() func() *time.Ticket { delimiter := e.executedAt.Delimiter() if contents != nil { delimiter += uint32(len(contents)) } return func() *time.Ticket { delimiter++ return time.NewTicket( e.executedAt.Lamport(), delimiter, e.executedAt.ActorID(), ) } }(), - e.maxCreatedAtMapByActor, options.VersionVector, )
Line range hint
93-125: Consider refactoring timeTicket handlingThe TODO comment indicates a need to refactor the timeTicket handling logic for multi-level concurrent editing. This complexity might affect the version vector implementation.
Consider:
- Documenting the relationship between timeTickets and version vectors
- Creating a separate issue to track the refactoring of the timeTicket simulation logic
- Adding tests to verify the behavior of concurrent multi-level editing with version vectors
Transition to version vectors is incomplete - maxCreatedAtMapByActor still in use
The field
maxCreatedAtMapByActoris still actively used across multiple core files in the codebase:
pkg/document/operations/tree_style.gopkg/document/operations/tree_edit.gopkg/document/operations/style.gopkg/document/operations/edit.gopkg/document/crdt/tree.gopkg/document/crdt/text.gopkg/document/crdt/rga_tree_split.goThe field is not only present in struct definitions but is actively used in method calls and operations, indicating that the transition to version vectors is incomplete. The design document (
design/tree.md) also references this field as part of the core functionality.🔗 Analysis chain
Line range hint
1-1: Verify complete removal of maxCreatedAtMapByActorTo ensure a clean transition to version vectors, we need to verify that all references to maxCreatedAtMapByActor are removed consistently across the codebase.
Let me gather more information to check for any usage or references of maxCreatedAtMapByActor.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining struct fields ast-grep --pattern 'struct { $$$ maxCreatedAtMapByActor $_ $$$ }' # Search for any remaining method parameters ast-grep --pattern 'func $_( $$$ maxCreatedAtMapByActor $_ $$$ )' # Search for any remaining variable declarations ast-grep --pattern 'maxCreatedAtMapByActor := $_'Length of output: 6463
Script:
#!/bin/bash # Search for any references to maxCreatedAtMapByActor in function calls or assignments rg "maxCreatedAtMapByActor" -A 2 -B 2 # Search for any methods that might be using this field ast-grep --pattern 'func $_ $_($$$) $_ { $$$ maxCreatedAtMapByActor $$$ }'Length of output: 12604
pkg/document/crdt/tree.go (2)
Line range hint
542-544: Create a database migration plan.The TODO comments indicate a need for database migration to support version vectors. This should be planned carefully to ensure data consistency during the transition.
Consider:
- Creating a migration script
- Adding version checks in the code
- Implementing a rollback strategy
- Testing the migration with production data volume
Would you like help creating a migration plan or tracking this in a separate issue?
982-991: Refactor duplicated version vector handling logic.The version vector handling logic is duplicated in multiple locations. Consider extracting this into a helper function.
+func getClientLamportAtChange(versionVector time.VersionVector, actorID string) int64 { + if versionVector == nil { + return time.MaxLamport + } + if lamport, ok := versionVector.Get(actorID); ok { + return lamport + } + return 0 +}This would simplify the code and reduce duplication across multiple methods.
Also applies to: 1042-1051
test/integration/gc_test.go (2)
1196-1288: Add documentation to clarify the test's objectives.While the test case is well-structured, it would benefit from a comment block explaining:
- The purpose of testing the complete document lifecycle
- The significance of garbage collection during detachment
- The expected behavior of version vectors
Add this documentation at the start of the test:
t.Run("attach > pushpull > detach lifecycle version vector test (run gc at last client detaches document)", func(t *testing.T) { + // This test verifies that: + // 1. Version vectors are properly maintained during document lifecycle + // 2. Garbage collection runs correctly when clients detach + // 3. The last client detaching triggers final garbage collection + // 4. All deleted nodes are properly collected after the last detachment clients := activeClients(t, 2)
Line range hint
1-1338: Consider reorganizing tests for better maintainability.The test file contains multiple related test cases that could be better organized using test groups. Consider using subtests to group related scenarios:
func TestGarbageCollection(t *testing.T) { + // Group 1: Basic GC operations t.Run("group=basic", func(t *testing.T) { t.Run("garbage collection for container type test", func(t *testing.T) {...}) t.Run("garbage collection for text type test", func(t *testing.T) {...}) t.Run("garbage collection for tree type test", func(t *testing.T) {...}) }) + // Group 2: Multi-client scenarios t.Run("group=multi-client", func(t *testing.T) { t.Run("garbage collection for tree type test (multi clients)", func(t *testing.T) {...}) t.Run("concurrent garbage collection test", func(t *testing.T) {...}) }) + // Group 3: Lifecycle tests t.Run("group=lifecycle", func(t *testing.T) { t.Run("attach > pushpull > detach lifecycle version vector test", func(t *testing.T) {...}) t.Run("detached client node deletion test", func(t *testing.T) {...}) }) }pkg/document/crdt/text.go (2)
333-334: Address TODO comment regarding database migrationThe TODO comment indicates a required database migration to support version vectors for existing changes. This should be addressed before completing the transition.
Would you like help creating:
- A database migration plan
- A GitHub issue to track this work
- Documentation for the migration process
Line range hint
276-335: Consider completing the version vector transition in a single PRThe current implementation maintains both versioning mechanisms in some places while transitioning to version vectors in others. This hybrid approach could lead to inconsistencies and make the codebase harder to maintain.
Consider:
- Completing the full transition to version vectors in this PR
- Creating a separate PR for the database migration
- Adding tests specifically for version vector behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
api/converter/from_bytes.go(1 hunks)pkg/document/change/change.go(1 hunks)pkg/document/crdt/rga_tree_split.go(6 hunks)pkg/document/crdt/root_test.go(4 hunks)pkg/document/crdt/text.go(3 hunks)pkg/document/crdt/text_test.go(2 hunks)pkg/document/crdt/tree.go(11 hunks)pkg/document/crdt/tree_test.go(1 hunks)pkg/document/document.go(2 hunks)pkg/document/internal_document.go(1 hunks)pkg/document/json/text.go(2 hunks)pkg/document/json/tree.go(3 hunks)pkg/document/operations/add.go(1 hunks)pkg/document/operations/array_set.go(1 hunks)pkg/document/operations/edit.go(1 hunks)pkg/document/operations/increase.go(1 hunks)pkg/document/operations/move.go(1 hunks)pkg/document/operations/operation.go(1 hunks)pkg/document/operations/remove.go(1 hunks)pkg/document/operations/set.go(1 hunks)pkg/document/operations/style.go(1 hunks)pkg/document/operations/tree_edit.go(2 hunks)pkg/document/operations/tree_style.go(2 hunks)pkg/document/time/version_vector.go(1 hunks)test/integration/gc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- pkg/document/operations/add.go
- pkg/document/time/version_vector.go
- pkg/document/operations/increase.go
- pkg/document/operations/array_set.go
- api/converter/from_bytes.go
- pkg/document/operations/move.go
- pkg/document/document.go
- pkg/document/operations/remove.go
- pkg/document/internal_document.go
- pkg/document/operations/set.go
- pkg/document/crdt/tree_test.go
- pkg/document/crdt/text_test.go
- pkg/document/json/text.go
- pkg/document/crdt/root_test.go
- pkg/document/change/change.go
- pkg/document/json/tree.go
- pkg/document/operations/operation.go
- pkg/document/operations/style.go
🔇 Additional comments (6)
pkg/document/operations/edit.go (1)
73-77: Verify the impact of Execute method changes
The Execute method signature has been updated to accept options. Let's verify all callers have been updated accordingly.
✅ Verification successful
Based on the search results, I can now generate the final response:
Execute method changes have been properly implemented across the codebase
The Execute method signature change has been consistently implemented across all operation types:
- All operation types (Edit, Style, TreeStyle, etc.) have been updated to accept variadic options parameter
- The changes are properly propagated through the call chain in
change.Change.Execute - Some operations explicitly ignore options using
_while others properly handle them based on their needs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to Execute method
rg -A 2 "Execute\(" --type go
# Search for any remaining references to maxCreatedAtMapByActor
rg "maxCreatedAtMapByActor" --type go
Length of output: 7103
pkg/document/crdt/tree.go (1)
384-390: Review Lamport timestamp comparison logic.
The condition n.id.CreatedAt.Lamport() <= clientLamportAtChange in canDelete might need revision based on previous review comments about timestamp comparisons.
#!/bin/bash
# Search for similar Lamport timestamp comparisons to verify consistency
rg -A 2 "Lamport\(\) [<=>]+ clientLamportAtChange"test/integration/gc_test.go (1)
1290-1338: The skipped test should be implemented.
The test case is important for verifying node deletion behavior, especially with detached clients. The implementation exists in pkg/document/crdt/rga_tree_split.go.
Would you like help implementing this test case? I can help:
- Complete the test implementation
- Add assertions for node deletion verification
- Add proper cleanup in the deferred function
pkg/document/crdt/rga_tree_split.go (2)
261-264:
Fix Logical Error in Remove Method Condition
The condition for node removal might not correctly handle all edge cases with Lamport timestamps.
Apply this diff to correct potential logical errors:
-func (s *RGATreeSplitNode[V]) Remove(removedAt *time.Ticket, clientLamportAtChange int64) bool {
- if (s.createdAt().Lamport() <= clientLamportAtChange) &&
- (s.removedAt == nil || removedAt.After(s.removedAt)) {
+func (s *RGATreeSplitNode[V]) Remove(removedAt *time.Ticket, clientLamportAtChange int64) bool {
+ if (s.createdAt().Lamport() < clientLamportAtChange) ||
+ (s.removedAt != nil && removedAt.Before(s.removedAt)) {527-545: Ensure consistent migration strategy across components
The TODO comments about database migration should align with the migration strategy in other components (e.g., Text.Style).
Consider:
- Creating a comprehensive migration plan that covers all affected components
- Adding tests to verify the migration process
- Documenting the migration steps for operators
Run this script to identify all components requiring migration:
pkg/document/crdt/text.go (1)
321-329: LGTM: Robust version vector handling
The implementation properly handles:
- Nil version vector case with MaxLamport fallback
- Missing actor IDs with 0 lamport time fallback
- Proper actor ID lookup and lamport time extraction
Add test case verifying behavior when client A attempts to delete nodes created by client B after client B is detached. This test is added to discuss on how to handle this scenario.
…lace-maxcreatedat
chacha912
force-pushed
the
replace-maxcreatedat
branch
from
683bfec to
e784fe5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
test/integration/gc_test.go (5)
1307-1307: Consider a more descriptive test nameThe test name could be more descriptive to better reflect what's being tested. Consider something like "verify_gc_behavior_during_client_lifecycle" or "gc_executes_after_final_client_detachment".
1382-1382: Address the TODO comment about version vector cleanupThe TODO comment indicates a need to handle cleanup of detached client's Lamport clock from the version vector. This should be addressed to prevent potential memory leaks from accumulating version vector entries.
Would you like me to help create a GitHub issue to track this technical debt?
1402-1449: Add detailed comments explaining the test scenarioThis test case verifies important functionality around node deletion with detached clients, but it would benefit from more detailed comments explaining:
- The purpose of each step in the test
- The expected behavior when a detached client's nodes are modified
- The expected state of the version vector at key points
Example comment structure:
// This test verifies that: // 1. Changes from a client are properly synchronized before detachment // 2. Node deletions after client detachment are handled correctly // 3. Version vectors are properly updated when operating on nodes from detached clients
1442-1442: Update the comment to match the actual operationThe comment "delete 123 and insert x" doesn't match the actual operation which only replaces "1" with "x". Update the comment to accurately reflect the code.
1307-1449: Consider restructuring tests for better isolationThe test cases verify complex distributed system behavior but could benefit from:
- Breaking down into smaller, more focused test cases that isolate specific behaviors
- Extracting common setup code into helper functions
- Adding test cases for edge cases (e.g., network partitions, concurrent detachments)
This would improve maintainability and make it easier to identify issues when tests fail.
pkg/document/crdt/rga_tree_split.go (3)
Line range hint
261-277: Consider improving variable naming and documentationThe variable
nodeExistedcould be renamed to better reflect its purpose, such asisNodeEligibleForRemovalorcanNodeBeRemoved. Additionally, the TODO comment about legacy support could be expanded to include migration timeline expectations.- var nodeExisted bool + var isNodeEligibleForRemoval bool - if maxCreatedAt == nil { - nodeExisted = s.createdAt().Lamport() <= clientLamportAtChange + if maxCreatedAt == nil { + isNodeEligibleForRemoval = s.createdAt().Lamport() <= clientLamportAtChange
Line range hint
470-502: Enhance error handling in the edit methodConsider wrapping errors with additional context to help with debugging. Use
fmt.Errorfto include operation details when returning errors.- return nil, nil, nil, err + return nil, nil, nil, fmt.Errorf("failed to find node with split for 'to' position: %w", err)
Line range hint
544-569: Improve version vector handling structureThe version vector handling logic could be clearer with a dedicated helper method. This would improve readability and make the code more maintainable.
+func getLamportFromVersionVector( + versionVector time.VersionVector, + maxCreatedAtMapByActor map[string]*time.Ticket, + actorID []byte, +) int64 { + if versionVector == nil && maxCreatedAtMapByActor == nil { + return time.MaxLamport + } + if versionVector != nil { + if lamport, ok := versionVector.Get(actorID); ok { + return lamport + } + return 0 + } + return 0 +}pkg/document/crdt/tree.go (1)
Line range hint
692-754: Document implications of nil version vector in EditTThe
EditTmethod passes nil for both version vector and maxCreatedAtMapByActor. This behavior should be explicitly documented to explain the implications for concurrent operations.Add a comment explaining the behavior:
+// EditT edits the given range with the given value. +// Note: This method is intended for testing and passes nil for both version vector +// and maxCreatedAtMapByActor, which means it will use MaxLamport for comparison. +// Do not use this method in production code where concurrent operation handling is required. func (t *Tree) EditT(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
api/converter/from_bytes.go(1 hunks)pkg/document/crdt/rga_tree_split.go(7 hunks)pkg/document/crdt/text.go(5 hunks)pkg/document/crdt/tree.go(13 hunks)pkg/document/time/version_vector.go(1 hunks)test/integration/gc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/converter/from_bytes.go
- pkg/document/time/version_vector.go
- pkg/document/crdt/text.go
🔇 Additional comments (1)
pkg/document/crdt/tree.go (1)
384-395: Ensure consistent version vector handling across types
The version vector handling should be consistent between Tree and RGATreeSplit. Consider creating a shared interface or utility package for version vector operations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
pkg/document/change/change.go(1 hunks)pkg/document/operations/add.go(1 hunks)pkg/document/operations/array_set.go(1 hunks)pkg/document/operations/edit.go(1 hunks)pkg/document/operations/increase.go(1 hunks)pkg/document/operations/move.go(1 hunks)pkg/document/operations/operation.go(1 hunks)pkg/document/operations/remove.go(1 hunks)pkg/document/operations/set.go(1 hunks)pkg/document/operations/style.go(1 hunks)pkg/document/operations/tree_edit.go(2 hunks)pkg/document/operations/tree_style.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/document/operations/add.go
- pkg/document/operations/move.go
- pkg/document/operations/remove.go
- pkg/document/operations/set.go
- pkg/document/operations/operation.go
- pkg/document/operations/array_set.go
- pkg/document/change/change.go
- pkg/document/operations/increase.go
- pkg/document/operations/edit.go
- pkg/document/operations/tree_style.go
🔇 Additional comments (4)
pkg/document/operations/style.go (1)
66-66: LGTM: Execute method signature updated correctly
The addition of the versionVector parameter aligns with the PR's objective to transition from maxCreatedAtMap to version vector for causal-concurrent operations.
pkg/document/operations/tree_edit.go (3)
72-72: LGTM: Execute method signature updated correctly
The addition of the versionVector parameter aligns with the PR's objective.
119-120: Verify: Potential incomplete refactoring detected
Similar to the Style operation, this code still uses both maxCreatedAtMapByActor and versionVector. Please clarify:
- Is this intentional for backward compatibility?
- If yes, should we add a comment explaining the dual usage?
- If no, should we remove maxCreatedAtMapByActor usage?
✅ Verification successful
Dual usage of maxCreatedAtMapByActor and versionVector is intentional
The code pattern is consistent across the codebase. Both parameters are used together for version control in CRDT operations, where:
- When both are nil, it's treated as a local edit
- When versionVector is present, it's used for remote operations
- They serve different purposes in the version control mechanism as evidenced by the consistent pattern in multiple files (text.go, tree.go, rga_tree_split.go)
A comment explaining this behavior already exists in the codebase: // Local edit - use version vector comparison
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Edit method calls to verify parameter usage pattern
ast-grep --pattern 'Edit($$$, maxCreatedAtMapByActor, versionVector)'
Length of output: 71
Script:
#!/bin/bash
# Let's first check the Edit method definition and its usage
ast-grep --pattern 'func $_ Edit($$$)'
# Also search for any usage of maxCreatedAtMapByActor and versionVector together
rg "maxCreatedAtMapByActor.*versionVector|versionVector.*maxCreatedAtMapByActor" -A 5 -B 5
Length of output: 7168
Line range hint 91-120: Consider refactoring the timeTicket generation logic
The TODO comment highlights potential issues with the current implementation:
- The timeTicket generation for split elements is complex and might be unclear
- The comment suggests need for refactoring multi-level concurrent editing
Consider:
- Extracting the ticket generation logic into a separate method
- Documenting the delimiter increment behavior more clearly
- Adding test cases for multi-level concurrent editing scenarios
hackerwins
changed the title
What this PR does / why we need it:
This PR replaces the usage of
MaxCreatedAtMapByActorwithversion vectorfor operations that handle causal and concurrent relationships. (Text.Edit,Text.Style,Tree.Edit, andTree.Style)The introduction of version vector allows us to track which client's Lamport time existed when a change was created. For example, during delete operations, we compare the Lamport time of the deleted character's creation with the Lamport time from the version vector to maintain proper concurrency handling.
As shown in the diagram, when checking if a node should be removed, we compare:
6@A)2@A)Which issue(s) this PR fixes:
Related #723 #1047
Special notes for your reviewer:
There are two issues with replacing maxCreatedAt with VV (Version Vector):
[Resolved] Modified to retain client information in change VV even after detachment, ensuring all client states remain traceable. (#1090)
[Resolved] Since the version vectors in existing data are uncertain, we can proceed after removing all this information. If a change has no version vector, maxCreatedAt will be used; otherwise, we use the version vector to verify nodeExisted.
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor