-
-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Concurrency Tests between Array Operations #985
Conversation
WalkthroughThe changes introduce a new operation type, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Document
participant Array
Client->>API: Request SetByIndex
API->>Document: Validate operation
Document->>Array: Set element at index
Array-->>Document: Acknowledge change
Document-->>API: Return success
API-->>Client: Respond with result
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (4)
pkg/document/crdt/rga_tree_list.go (1)
349-354
: Add documentation for theSetByIndex
method.Consider adding a docstring to explain the purpose and usage of the
SetByIndex
method.pkg/document/json/array.go (2)
170-173
: Add documentation for theMoveAfterByIndex
method.Consider adding a docstring to explain the purpose and usage of the
MoveAfterByIndex
method.
311-321
: Add documentation for theSetInteger
method.Consider adding a docstring to explain the purpose and usage of the
SetInteger
method.api/converter/to_pb.go (1)
380-394
: Add documentation for thetoSetByIndex
function.Consider adding a docstring to explain the purpose and usage of the
toSetByIndex
function.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
api/yorkie/v1/resources.pb.go
is excluded by!**/*.pb.go
Files selected for processing (11)
- api/converter/from_pb.go (2 hunks)
- api/converter/to_pb.go (2 hunks)
- api/docs/yorkie/v1/admin.openapi.yaml (2 hunks)
- api/docs/yorkie/v1/resources.openapi.yaml (2 hunks)
- api/docs/yorkie/v1/yorkie.openapi.yaml (2 hunks)
- api/yorkie/v1/resources.proto (2 hunks)
- pkg/document/crdt/array.go (1 hunks)
- pkg/document/crdt/rga_tree_list.go (1 hunks)
- pkg/document/json/array.go (3 hunks)
- pkg/document/operations/set_by_index.go (1 hunks)
- test/integration/array_test.go (1 hunks)
Additional comments not posted (28)
pkg/document/operations/set_by_index.go (10)
1-15
: LGTM!The license header is correctly formatted.
17-22
: LGTM!The package declaration and imports are correctly formatted.
24-37
: LGTM!The
SetByIndex
struct definition is correctly formatted and includes necessary fields.
39-52
: LGTM!The
NewSetByIndex
constructor function is correctly implemented.
54-80
: LGTM! But address the commented-out code.The
Execute
method is correctly implemented. However, the commented-out code for GC-like logic should be addressed or removed if not needed.
82-85
: LGTM!The
Value
method is correctly implemented.
87-90
: LGTM!The
ParentCreatedAt
method is correctly implemented.
92-95
: LGTM!The
ExecutedAt
method is correctly implemented.
97-100
: LGTM!The
SetActor
method is correctly implemented.
102-105
: LGTM!The
CreatedAt
method is correctly implemented.pkg/document/crdt/array.go (3)
Line range hint
1-15
: LGTM!The license header is correctly formatted.
Line range hint
17-22
: LGTM!The package declaration and imports are correctly formatted.
207-217
: LGTM!The
SetByIndex
method is correctly implemented.api/yorkie/v1/resources.proto (3)
Line range hint
1-15
: LGTM!The license header is correctly formatted.
139-145
: LGTM!The
SetByIndex
message definition is correctly formatted and includes necessary fields.
158-158
: LGTM!The update to the
oneof body
section is correctly implemented.pkg/document/crdt/rga_tree_list.go (1)
360-369
: Clarify the TODO comment and update timestamp usage.The TODO comment suggests using
UpdatedAt
instead ofMovedAt
. Ensure that the correct timestamp is used for the intended logic.Verify if
UpdatedAt
should be used instead ofMovedAt
and update the code accordingly.api/converter/to_pb.go (1)
386-393
: Improve error message clarity.The error message "toSetByIndex %s: %w" could be more descriptive to indicate the specific error.
Apply this diff to improve the error message:
- return nil, fmt.Errorf("toSetByIndex %s: %w", setByIndex, err) + return nil, fmt.Errorf("toSetByIndex: failed to convert value for setByIndex operation: %w", err)Likely invalid or redundant comment.
api/converter/from_pb.go (1)
544-567
: LGTM!The function
fromSetByIndex
is correctly implemented. It handles the conversion of all necessary fields and manages errors appropriately.test/integration/array_test.go (3)
475-568
: LGTM!The function
TestArrayConcurrencyTable
is well-structured and comprehensive. It systematically tests various operations on an array in a concurrent context, ensuring consistency in the results.
570-608
: LGTM!The function
TestArraySetByIndex
is correctly implemented. It includes detailed test cases and assertions to validate the expected state of the arrays after operations.
610-762
: LGTM!The function
TestArrayConcurrency
is correctly implemented. It includes detailed test cases and assertions to validate the expected state of the arrays after operations.api/docs/yorkie/v1/yorkie.openapi.yaml (2)
703-708
: LGTM!The addition of
setByIndex
underyorkie.v1.Operation
is correctly formatted and follows the existing pattern for other operations.
978-1007
: LGTM!The definition of
yorkie.v1.Operation.SetByIndex
is correctly formatted and follows the existing pattern for other operations.api/docs/yorkie/v1/resources.openapi.yaml (2)
719-724
: LGTM!The addition of
setByIndex
in theyorkie.v1.Operation
schema is consistent with the existing structure of other operations.
994-1023
: LGTM!The definition of the
SetByIndex
schema is correct and follows the existing pattern for other operation schemas.api/docs/yorkie/v1/admin.openapi.yaml (2)
1210-1215
: LGTM!The addition of the
setByIndex
property to theyorkie.v1.Operation
schema is consistent with the introduction of the newSetByIndex
operation.
1485-1514
: LGTM!The definition of the
yorkie.v1.Operation.SetByIndex
schema is well-structured and consistent with the existing schemas. The description provides useful context for thevalue.created_at
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
pkg/document/json/array.go (1)
416-447
: Review the implementation of setByIndexInternal.This internal helper function supports the
SetInteger
method by handling the creation of a new element and its insertion into the array. The function correctly issues a time ticket, creates a new primitive element, and registers the set operation in the context. However, there is a commented-out section related to garbage collection logic, which suggests potential future enhancements or considerations.Consider finalizing or removing the commented-out code related to garbage collection to clean up the implementation and avoid confusion.
- // if removed != nil { - // // TODO(junseo): add GC-like logic - // // p.context.RegisterRemovedElementPair(p, removed) - // }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- pkg/document/json/array.go (9 hunks)
Additional comments not posted (1)
pkg/document/json/array.go (1)
401-414
: Review the implementation of moveAfterInternal.This internal helper function is used to move elements within the array based on their creation timestamps. The implementation correctly issues a time ticket and registers the move operation in the context. Error handling is present, ensuring that any issues during the move operation are caught and handled appropriately.
The code changes are approved.
What is intended behavior for invalid index? In json array, GetObject, GetArray, ... , Delete method validate index itself and call func (p *Array) InsertIntegerAfter(index int, v int) *Array {
p.insertAfterInternal(p.Get(index).CreatedAt(), func(ticket *time.Ticket) crdt.Element {
primitive, err := crdt.NewPrimitive(v, ticket)
if err != nil {
panic(err)
}
return primitive
})
return p
} Update) Refactor index validation in json array in cef5b8f
This might be related to PR #524, so I added panic method only in scenarios where a 'nil pointer dereference' occurs. |
Delegate index validation to Get and Delete methods. GetObject, GetArray, GetText, GetCounter, GetTree return 'nil' for invalid index. InsertIntegerAfter, MoveAfterByIndex, SetInteger panic with "index out of bound" for invalid index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution🚀. I've left a few comments, please check them.
- Add note for array concurrency table tests. - Expand the testing coverage from the upper diagonal to the entire matrix. - Remove duplicated tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- pkg/document/crdt/rga_tree_list.go (1 hunks)
- pkg/document/json/array.go (9 hunks)
- pkg/document/operations/set_by_index.go (1 hunks)
- test/integration/array_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- pkg/document/crdt/rga_tree_list.go
- pkg/document/json/array.go
- pkg/document/operations/set_by_index.go
Additional comments not posted (2)
test/integration/array_test.go (2)
475-569
: Comprehensive concurrency testing for array operations. LGTM!The
TestArrayConcurrencyTable
function provides thorough testing of concurrent array operations:
- Covers key operations: insert, move, set, remove
- Tests all pairs of operations
- Verifies consistency of array state across clients
- Well-structured with the use of
arrayOp
struct andrunTest
helperThe test code looks good to me. Nice work!
571-608
: Good test for setting array elements by index concurrently. Approved!The
TestArraySetByIndex
function tests an important scenario of setting array elements by index concurrently from multiple clients.The test initializes the array and then concurrently sets values at two different indices. The assertions verify that the final array state is as expected and consistent across the clients.
Overall, this is a well-written test case. No issues found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good to me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
If both tests are testing the same case, it may be good to remove first one.
The test code isn't very big, so I think it's okay for it to be in the same file for now. |
A. Total Tests: 49 cases(7*7) Initially, the symmetric relationship between clients in operation combinations was not included. However, since the Lamport clock comparison varies depending on the client ID, the tests were modified to check all cases. B. Operations per Client: 7(2+3+1+1) - Insert: 2 cases(Prev, Prev.Next) - Move: 3 cases(Prev, Prev.Next, Target) - Set: 1 case(Target) - Remove: 1 case(Target) C. Failure Cases: 10 cases(2*2 + 2 + 1*2 + 1*2) 1. *.Prev == Move.Target 4 cases(2*2, symmetric) - Insert.Prev == Move.Target - Move.Prev == Move.Target 2. *.Prev.Next == Set.Target: 2 cases(asymmetric) - Insert.Prev.Next == Set.Target - Move.Prev.Next == Set.Target 3. Move.Target == Set.Target: 2 cases(1*2, symmetric, client ID matters) 4. Remove.Target == Set.Target: 2 cases (1*2, symmetric)
What this PR does / why we need it:
Implement Array.SetByIndex operation partially.
Add conccurency tests between array operations(Insert, Move, Set, Remove).
Problem Space Definition / Test Implementation
Problem Space Definition
Previous
Element,Previous.Next
ElementPrevious
Element,Previous.Next
Element,Target
ElementTarget
ElementTarget
Elementop1.x == op2.y
(7 * 7)Test Implementation
op1.x == op2.y == index k
and fix k (==oneIdx
)operations
arrayTests (7 * 7 = 49 cases)
Failure cases (2*2 + 2 + 1*2 + 1*2 = 10 cases)
Why failure occurs
Set
operation updatesmovedAt
.updatedAt
required.Set
operation updatesmovedAt
.updatedAt
required.Set
operation does not copyremovedAt
of old element.Which issue(s) this PR fixes:
Fixes yorkie-team/yorkie-js-sdk#191
Special notes for your reviewer:
I initially created the hand-written simple test cases in
func TestArrayConcurrency
. To ensure all cases are covered, I also added tests for all operation pairs infunc TestArrayConcurrencyTable
.The former can be reproduced in the latter. Would it be better to delete the former tests? And would it be better to create a new test file for array concurrency, similar to
tree_concurrency_test.go
?Additional documentation:
Future works: (#990)
TestArrayConcurrencyTable
. (currently, comparedoc.Marshal()
only)updatedAt
inElement
interface.Checklist:
Summary by CodeRabbit
New Features
SetByIndex
operation for enhanced array manipulation, allowing elements to be set at specific indices.MoveAfterByIndex
andSetInteger
to theArray
type for improved element repositioning and integer value setting.Bug Fixes
Tests