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 (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 theSetByIndexmethod.Consider adding a docstring to explain the purpose and usage of the
SetByIndexmethod.pkg/document/json/array.go (2)
170-173: Add documentation for theMoveAfterByIndexmethod.Consider adding a docstring to explain the purpose and usage of the
MoveAfterByIndexmethod.
311-321: Add documentation for theSetIntegermethod.Consider adding a docstring to explain the purpose and usage of the
SetIntegermethod.api/converter/to_pb.go (1)
380-394: Add documentation for thetoSetByIndexfunction.Consider adding a docstring to explain the purpose and usage of the
toSetByIndexfunction.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
api/yorkie/v1/resources.pb.gois 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
SetByIndexstruct definition is correctly formatted and includes necessary fields.
39-52: LGTM!The
NewSetByIndexconstructor function is correctly implemented.
54-80: LGTM! But address the commented-out code.The
Executemethod is correctly implemented. However, the commented-out code for GC-like logic should be addressed or removed if not needed.
82-85: LGTM!The
Valuemethod is correctly implemented.
87-90: LGTM!The
ParentCreatedAtmethod is correctly implemented.
92-95: LGTM!The
ExecutedAtmethod is correctly implemented.
97-100: LGTM!The
SetActormethod is correctly implemented.
102-105: LGTM!The
CreatedAtmethod 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
SetByIndexmethod 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
SetByIndexmessage definition is correctly formatted and includes necessary fields.
158-158: LGTM!The update to the
oneof bodysection 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
UpdatedAtinstead ofMovedAt. Ensure that the correct timestamp is used for the intended logic.Verify if
UpdatedAtshould be used instead ofMovedAtand 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
fromSetByIndexis 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
TestArrayConcurrencyTableis 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
TestArraySetByIndexis correctly implemented. It includes detailed test cases and assertions to validate the expected state of the arrays after operations.
610-762: LGTM!The function
TestArrayConcurrencyis 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
setByIndexunderyorkie.v1.Operationis correctly formatted and follows the existing pattern for other operations.
978-1007: LGTM!The definition of
yorkie.v1.Operation.SetByIndexis correctly formatted and follows the existing pattern for other operations.api/docs/yorkie/v1/resources.openapi.yaml (2)
719-724: LGTM!The addition of
setByIndexin theyorkie.v1.Operationschema is consistent with the existing structure of other operations.
994-1023: LGTM!The definition of the
SetByIndexschema 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
setByIndexproperty to theyorkie.v1.Operationschema is consistent with the introduction of the newSetByIndexoperation.
1485-1514: LGTM!The definition of the
yorkie.v1.Operation.SetByIndexschema is well-structured and consistent with the existing schemas. The description provides useful context for thevalue.created_atproperty.
cloneot
changed the title
hackerwins
requested review from
hackerwins,
sejongk,
chacha912 and
raararaara
There was a problem hiding this comment.
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
SetIntegermethod 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.
- 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.
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
TestArrayConcurrencyTablefunction 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
arrayOpstruct andrunTesthelperThe test code looks good to me. Nice work!
571-608: Good test for setting array elements by index concurrently. Approved!The
TestArraySetByIndexfunction 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.
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
PreviousElement,Previous.NextElementPreviousElement,Previous.NextElement,TargetElementTargetElementTargetElementop1.x == op2.y(7 * 7)Test Implementation
op1.x == op2.y == index kand fix k (==oneIdx)operationsarrayTests (7 * 7 = 49 cases)
Failure cases (2*2 + 2 + 1*2 + 1*2 = 10 cases)
Why failure occurs
Setoperation updatesmovedAt.updatedAtrequired.Setoperation updatesmovedAt.updatedAtrequired.Setoperation does not copyremovedAtof 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)updatedAtinElementinterface.Checklist:
Summary by CodeRabbit
New Features
SetByIndexoperation for enhanced array manipulation, allowing elements to be set at specific indices.MoveAfterByIndexandSetIntegerto theArraytype for improved element repositioning and integer value setting.Bug Fixes
Tests