GO-2827 Unbind user files (reclaim / reconcile space usage) #1226
Conversation
GO-3441: Delete uploading files that were deleted
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files ignored due to path filters (1)
Files selected for processing (5)
WalkthroughThe changes introduce new interfaces and methods for file storage and reconciliation, enhancing the system's file handling and synchronization capabilities. Key updates include adding the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant Reconciler
participant FileStorage
Client->>Middleware: FileReconcile(ctx, req)
Middleware->>Reconciler: Start()
Reconciler->>FileStorage: IterateFiles(ctx, iterFunc)
FileStorage-->>Reconciler: File IDs
Reconciler-->>Middleware: Reconciliation Status
Middleware-->>Client: FileReconcile Response
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 23
Out of diff range and nitpick comments (10)
core/syncstatus/filestatus.go (1)
Line range hint
25-45: The methodindexFileSyncStatushandles the status update logic. It's good to see error handling and the use of interfaces for setting details. However, the error messages could be more descriptive to better indicate the source of the error.- return fmt.Errorf("get object: %w", err) + return fmt.Errorf("failed to get object in indexFileSyncStatus: %w", err)core/syncstatus/service.go (1)
92-92: Consider adding a comment here to clarify that this method is intentionally left empty, if that's the case.core/filestorage/rpcstore/client.go (1)
131-150: Optimize the handling of file streaming initerateSpaceFiles.Consider handling large file lists more efficiently, possibly by processing in batches or asynchronously.
core/filestorage/filesync/upload.go (3)
75-75: Consider adding a comment explaining the conditions under whichrunOnLimitedHookis called.
Line range hint
90-117: Ensure proper error handling and logging inuploadingHandler. It seems that the function could benefit from more detailed logging, especially in the error paths to aid in debugging.
Line range hint
137-154: Similar to theuploadingHandler, theretryingHandlershould also include more detailed error logging. Consider refactoring to share common logic withuploadingHandlerto reduce code duplication.core/filestorage/filesync/mock_filesync/mock_FileSync.go (1)
Line range hint
844-887: Review the method signature and error handling inUploadSynchronously.The method
UploadSynchronouslyshould ensure that the context passed is not nil before proceeding with operations, as using a nil context can lead to runtime panics in some scenarios.+ if ctx == nil { + return errors.New("context cannot be nil") + }Additionally, consider handling potential nil values for
spaceIdandfileIdto prevent runtime errors.core/block/editor/smartblock/smartblock.go (3)
Line range hint
552-1003: Ensure comprehensive unit tests cover all branches and error conditions in theApplymethod.The
Applymethod contains multiple conditional branches and error handling paths. It would be beneficial to have a comprehensive set of unit tests to ensure all scenarios are correctly handled. Would you like assistance in setting up these tests?
Line range hint
552-1003: Add detailed documentation for the flags used in theApplymethod.The use of various flags in the
Applymethod controls significant aspects of its behavior. Adding detailed documentation explaining the purpose and effect of each flag can help maintainers and other developers understand the intended behavior of the method more quickly and accurately.
1003-1003: Clarify the operations performed in theStateRebuildmethod with inline comments.The
StateRebuildmethod performs several operations whose purposes are not immediately clear from the code. Adding inline comments explaining each significant step or operation within this method can enhance its readability and maintainability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.gopb/commands.pb.gois excluded by!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go
Files selected for processing (39)
- .mockery.yaml (1 hunks)
- core/anytype/bootstrap.go (2 hunks)
- core/block/delete.go (2 hunks)
- core/block/editor/factory.go (3 hunks)
- core/block/editor/files.go (3 hunks)
- core/block/editor/smartblock/smartblock.go (4 hunks)
- core/domain/errors.go (1 hunks)
- core/file.go (2 hunks)
- core/files/fileobject/service.go (3 hunks)
- core/files/files_test.go (3 hunks)
- core/files/reconciler/reconciler.go (1 hunks)
- core/files/reconciler/reconciler_test.go (1 hunks)
- core/filestorage/fileservice.go (3 hunks)
- core/filestorage/filesync/filesync.go (4 hunks)
- core/filestorage/filesync/filesync_test.go (2 hunks)
- core/filestorage/filesync/mock_filesync/mock_FileSync.go (4 hunks)
- core/filestorage/filesync/remove.go (5 hunks)
- core/filestorage/filesync/upload.go (8 hunks)
- core/filestorage/filesync/upload_test.go (1 hunks)
- core/filestorage/inmemory.go (2 hunks)
- core/filestorage/mock_filestorage/mock_FileStorage.go (1 hunks)
- core/filestorage/proxystore.go (2 hunks)
- core/filestorage/proxystore_test.go (6 hunks)
- core/filestorage/rpcstore/client.go (2 hunks)
- core/filestorage/rpcstore/inmemory.go (1 hunks)
- core/filestorage/rpcstore/mock_rpcstore/mock_rpcstore.go (1 hunks)
- core/filestorage/rpcstore/store.go (2 hunks)
- core/invitestore/invitestore.go (1 hunks)
- core/syncstatus/filestatus.go (1 hunks)
- core/syncstatus/service.go (1 hunks)
- docs/proto.md (5 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- space/spacecore/storage/sqlitestorage/space.go (1 hunks)
- space/spacecore/storage/sqlitestorage/sql.go (3 hunks)
- util/keyvaluestore/store.go (1 hunks)
- util/keyvaluestore/store_test.go (1 hunks)
- util/persistentqueue/queue.go (7 hunks)
- util/persistentqueue/queue_test.go (1 hunks)
Files not reviewed due to errors (4)
- core/files/files_test.go (no review received)
- core/filestorage/rpcstore/inmemory.go (no review received)
- core/filestorage/rpcstore/mock_rpcstore/mock_rpcstore.go (no review received)
- core/anytype/bootstrap.go (no review received)
Files skipped from review due to trivial changes (2)
- core/domain/errors.go
- core/filestorage/mock_filestorage/mock_FileStorage.go
Additional comments not posted (49)
util/keyvaluestore/store_test.go (1)
11-66: The test suite is well-structured and covers the basic functionalities of the key-value store. It's good to see the use ofrequirefor preconditions andassertfor postconditions, which is a best practice in testing.util/keyvaluestore/store.go (1)
42-48: The methodGetcorrectly handles not found errors and returns a custom error. This is a good practice as it abstracts the underlying database details from the caller.core/block/editor/files.go (2)
23-28: The initialization of theFilestruct is clear and well-structured. Each component is properly initialized, and the use of interfaces for components likeChangeReceiverandAllOperationsis a good practice for modularity and testing.
77-78: The methodInitcorrectly sets up hooks for file operations. However, ensure that theFileObjectHookmethod is robust and handles all edge cases since it's critical for file operations.Verification successful
The
FileObjectHookmethod in thereconcilerclass appears to incorporate robust error handling and manages several edge cases effectively. It checks if the reconciler is operational and handles potential errors from theneedToRebindmethod by wrapping and returning them, which is a good practice in Go for maintaining error traceability.However, the method returns
nilwhen the reconciler is not running, which might be intended behavior, assuming no operations should be performed when the reconciler is inactive. This could be clarified in the documentation or by further inspecting the usage context of this method to ensure it aligns with the overall system design.
- File:
core/files/reconciler/reconciler.go- Method:
FileObjectHookScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify robustness of FileObjectHook method. # Test: Search for the method implementation and check for error handling and edge cases. rg --type go $'FileObjectHook'Length of output: 916
Script:
#!/bin/bash # Description: Extract the implementation of FileObjectHook to assess error handling and edge case management. # Extract the method implementation from reconciler.go rg --type go -A 20 'func \(r \*reconciler\) FileObjectHook' core/files/reconciler/reconciler.goLength of output: 847
core/filestorage/filesync/filesync_test.go (1)
98-102: The methodwaitEmptyQueueis a utility function for testing queue operations. It's good to see the use of a timeout to prevent tests from hanging indefinitely. This is a best practice in writing reliable and maintainable tests.core/invitestore/invitestore.go (1)
84-84: The methodStoreInvitecorrectly handles the encryption and storage of invites. However, ensure that the encryption keys are securely managed and not exposed to logging or unauthorized access.core/filestorage/filesync/remove.go (2)
13-28: ThedeletionQueueItemstruct and its methods are well-defined. TheValidatemethod ensures that the file ID is valid before proceeding with deletion, which is a crucial check for data integrity.
73-73: TheretryDeletionHandlermethod handles retry logic for file deletions. Ensure that the retry mechanism does not lead to infinite loops or excessive resource usage in case of persistent errors.core/syncstatus/service.go (2)
88-90: Initialization of file upload event handlers looks good.
Line range hint
104-112: Proper locking mechanism around the map access inRegisterSpace.core/filestorage/fileservice.go (2)
116-118: Implementation ofIterateFilesmethod correctly delegates to the proxy object.
Line range hint
56-76: Initialization of file storage paths and components inInitmethod is robust and well-handled.space/spacecore/storage/sqlitestorage/sql.go (2)
Line range hint
24-41: SQL table definitions fortreesandtreeDeleteStatusesare correctly structured and safe withIF NOT EXISTS.
69-74: Preparation of SQL statements ininitStmtsmethod is correctly handled with appropriate error checking.core/block/delete.go (2)
70-72: Handling of different smart block types inDeleteObjectByFullIDmethod is clear and well-structured.
138-138: Enhanced logging in theOnDeletemethod improves traceability of delete operations..mockery.yaml (1)
162-164: Configuration for mock generation in.mockery.yamlis comprehensive and well-organized.core/filestorage/proxystore_test.go (4)
23-23: Test case for adding blocks to the cache store is well-structured and covers the necessary assertions.
39-39: Comprehensive test case for retrieving blocks from the cache store, covering both local and remote scenarios.
90-90: Effective use of channels and timeout in theTestCacheStore_GetManytest case ensures proper handling of asynchronous operations.
157-157: Test case for deleting blocks from the cache store correctly verifies the absence of blocks post-deletion.core/filestorage/filesync/filesync.go (3)
108-109: Initialization of persistent queues for file operations inInitmethod enhances reliability and performance.
118-118: Registration of upload completion callbacks inOnUploadedmethod allows for flexible and extensible event handling.
Line range hint
176-176: Thorough cleanup and shutdown process in theClosemethod ensures proper resource management.core/filestorage/proxystore.go (2)
76-79: Well-designed retrieval logic inGetmethod with appropriate use of context values to control behavior.
Line range hint
176-176: Proper resource management in theClosemethod ensures all stores are appropriately shut down.core/block/editor/factory.go (2)
62-62: The addition offileReconcilertoObjectFactoryaligns with the PR's objective to enhance file handling capabilities.
90-90: Initialization offileReconcilerusingapp.MustComponentensures that the component is mandatory for the application's operation, which is crucial given its role in file reconciliation.space/spacecore/storage/sqlitestorage/space.go (1)
183-183: The methodSetTreeDeletedStatuscorrectly updates the deletion status of a tree, which is essential for managing space storage states effectively.core/file.go (1)
199-210: TheFileReconcilefunction correctly starts the reconciliation process and handles errors appropriately, aligning with the PR's objectives to enhance file handling.core/filestorage/filesync/upload_test.go (1)
102-102: The test scenario correctly simulates the condition where an upload is attempted for a deleted file object, ensuring robustness in error handling.core/files/reconciler/reconciler_test.go (2)
125-127: The testTestReconcileRemoteStorageeffectively checks the reconciliation process against remote storage, ensuring that only the necessary files are deleted based on their status.
180-206: The test scenario underreconcilation startedcorrectly handles the condition where a file has been deleted, ensuring it is pushed to the rebinding queue. This is crucial for maintaining consistency in file status across the system.core/files/reconciler/reconciler.go (10)
38-43: TheReconcilerinterface is well-defined, encapsulating essential functionalities for file reconciliation.
45-57: Thereconcilerstruct is well-organized, encapsulating dependencies and state needed for the reconciliation process.
72-74: TheNewfunction correctly initializes a newreconcilerinstance. It's concise and follows best practices for factory methods.
106-116: TheRunmethod correctly checks if the reconciler has started and handles errors appropriately. The locking mechanism is used effectively to ensure thread safety.
119-128: TheStartmethod effectively sets theisStartedstate and handles errors. The use of locking to protect shared state is appropriate.
137-151: TheFileObjectHookmethod is well-implemented, providing a clear and concise way to handle file object hooks based on the reconciliation state and conditions.
154-166: TheneedToRebindmethod correctly checks the conditions under which a file needs rebinding. The logic is clear and the error handling is appropriate.
183-187: ThemarkAsReconciledmethod is concise and correctly updates the state based on the running status. The method's simplicity and directness are commendable.
190-232: ThereconcileRemoteStoragemethod is comprehensive, handling complex logic for reconciling remote storage. The error handling and logging are well-implemented, providing good visibility into the process.
235-241: TheClosemethod is straightforward and effectively closes therebindQueue. This is a clean and safe way to release resources.core/filestorage/filesync/upload.go (2)
190-205: The methodrunOnUploadedHookuses a loop to execute hooks and collects errors. Ensure that the error handling strategy is consistent and consider logging all errors, not just the first.Verification successful
The method
runOnUploadedHookin thefileSyncclass correctly executes all hooks in the loop, even if one of them fails, as evidenced by the absence of anybreakorreturnstatements within the loop that would terminate the execution prematurely. The error handling strategy involves collecting all errors into amultierror.Errorobject and selectively logging errors that are not identified as "object deleted" errors. This selective logging could be a deliberate design choice to avoid cluttering the logs with expected, handled errors.Given this, the review comment's suggestion to consider logging all errors might not align with the intended error handling strategy, which seems to be designed to log only significant, unexpected errors.
- The loop correctly executes all hooks, ensuring that the failure of one does not prevent the execution of others.
- The error logging is selective, likely to avoid logging known, handled errors.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all hooks in `runOnUploadedHook` are executed even if one fails. ast-grep --lang go --pattern $'func (s *fileSync) runOnUploadedHook($_, $_) error { $$$ for $_, $_ := range $_ { $_ := $_($_, $_) if $_ { $$$ } $$$ } $$$ }'Length of output: 1433
225-235: ForrunOnLimitedHook, similar to other hook methods, ensure that all hooks are attempted and errors are logged appropriately.core/files/fileobject/service.go (1)
81-81: Theservicestruct has been updated to includefileStorage. Ensure that this new dependency is properly initialized and used throughout the service methods to avoid runtime errors.pb/protos/service/service.proto (1)
151-151: New RPC methodFileReconcileadded to handle file reconciliation requests.This addition aligns with the PR's objectives to enhance file management and space optimization. Ensure that the corresponding server-side implementation handles the expected inputs and outputs correctly.
pb/protos/commands.proto (1)
3047-3062: The addition of theReconcilemessage in theFileservice is well-defined and consistent with the existing RPC structure and error handling conventions.docs/proto.md (1)
1883-1884: LGTM! The table entries are correctly formatted.
| // CancelDeletion provides a mock function with given fields: objectId, fileId | ||
| func (_m *MockFileSync) CancelDeletion(objectId string, fileId domain.FullFileId) error { | ||
| ret := _m.Called(objectId, fileId) | ||
|
|
||
| if len(ret) == 0 { | ||
| panic("no return value specified for CancelDeletion") | ||
| } | ||
|
|
||
| var r0 error | ||
| if rf, ok := ret.Get(0).(func(string, domain.FullFileId) error); ok { | ||
| r0 = rf(objectId, fileId) | ||
| } else { | ||
| r0 = ret.Error(0) | ||
| } | ||
|
|
||
| return r0 | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in CancelDeletion.
The method CancelDeletion uses a panic for handling the case where no return value is specified. This could lead to abrupt termination of the application in a testing scenario. Consider returning a default error instead to make the failure mode less severe.
- panic("no return value specified for CancelDeletion")
+ return errors.New("no return value specified for CancelDeletion")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // CancelDeletion provides a mock function with given fields: objectId, fileId | |
| func (_m *MockFileSync) CancelDeletion(objectId string, fileId domain.FullFileId) error { | |
| ret := _m.Called(objectId, fileId) | |
| if len(ret) == 0 { | |
| panic("no return value specified for CancelDeletion") | |
| } | |
| var r0 error | |
| if rf, ok := ret.Get(0).(func(string, domain.FullFileId) error); ok { | |
| r0 = rf(objectId, fileId) | |
| } else { | |
| r0 = ret.Error(0) | |
| } | |
| return r0 | |
| } | |
| // CancelDeletion provides a mock function with given fields: objectId, fileId | |
| func (_m *MockFileSync) CancelDeletion(objectId string, fileId domain.FullFileId) error { | |
| ret := _m.Called(objectId, fileId) | |
| if len(ret) == 0 { | |
| return errors.New("no return value specified for CancelDeletion") | |
| } | |
| var r0 error | |
| if rf, ok := ret.Get(0).(func(string, domain.FullFileId) error); ok { | |
| r0 = rf(objectId, fileId) | |
| } else { | |
| r0 = ret.Error(0) | |
| } | |
| return r0 | |
| } |
| @@ -550,7 +549,7 @@ func (sb *smartBlock) EnabledRelationAsDependentObjects() { | |||
| func (sb *smartBlock) Apply(s *state.State, flags ...ApplyFlag) (err error) { | |||
| startTime := time.Now() | |||
| if sb.IsDeleted() { | |||
| return ErrIsDeleted | |||
| return domain.ErrObjectIsDeleted | |||
There was a problem hiding this comment.
Consider refactoring the Apply method to improve readability and maintainability.
The Apply method is quite lengthy and handles multiple responsibilities. It would be beneficial to break this method into smaller, more focused methods. This can improve readability, make the code easier to maintain, and simplify unit testing.
| @@ -140,7 +161,7 @@ func (q *Queue[T]) handleNext() error { | |||
| if err != nil { | |||
| return fmt.Errorf("wait one: %w", err) | |||
| } | |||
| ok := q.Has(it.Key()) | |||
| ok := q.checkExistsAndMarkAsProcessing(it.Key()) | |||
There was a problem hiding this comment.
Ensure proper error handling when an item does not exist in the queue.
- return errRemoved
+ return fmt.Errorf("item does not exist in the queue: %w", errRemoved)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ok := q.checkExistsAndMarkAsProcessing(it.Key()) | |
| ok := q.checkExistsAndMarkAsProcessing(it.Key()) |
| @@ -149,11 +170,16 @@ | |||
| atomic.AddUint32(&q.handledItems, 1) | |||
| switch action { | |||
| case ActionDone: | |||
| removeErr := q.Remove(it.Key()) | |||
| removeErr := q.removeAndNotifyWaiters(it.Key()) | |||
There was a problem hiding this comment.
Consider logging the error when failing to remove an item from the queue.
+ q.logger.Error("Failed to remove item from queue", zap.Error(removeErr))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| removeErr := q.removeAndNotifyWaiters(it.Key()) | |
| removeErr := q.removeAndNotifyWaiters(it.Key()) | |
| q.logger.Error("Failed to remove item from queue", zap.Error(removeErr)) |
| func (s *store) IterateFiles(ctx context.Context, iterFunc func(fileId domain.FullFileId)) error { | ||
| _, err := writeOperation(s.backgroundCtx, ctx, s, "iterateFiles", func(c *client) (struct{}, error) { | ||
| return struct{}{}, c.iterateFiles(ctx, iterFunc) | ||
| }) | ||
| return err | ||
| } |
There was a problem hiding this comment.
The implementation of IterateFiles in RpcStore uses a write operation to iterate over files, which might not be optimal. Consider using a read operation unless a write is explicitly needed for each file iteration.
| t.Run("Remove: item is deleted after processing has started", func(t *testing.T) { | ||
| testCase := func(t *testing.T, action Action) { | ||
| t.Run(fmt.Sprintf("action: %s", action), func(t *testing.T) { | ||
| var q *Queue[*testItem] | ||
| db := newInMemoryBadger(t) | ||
| q = newTestQueueWithDb(db, func(ctx context.Context, item *testItem) (Action, error) { | ||
| err := q.Remove(item.Key()) | ||
| require.NoError(t, err) | ||
| return action, nil | ||
| }) | ||
|
|
||
| err := q.Add(&testItem{Id: "1", Timestamp: 1, Data: "data1"}) | ||
| require.NoError(t, err) | ||
|
|
||
| err = q.handleNext() | ||
| require.NoError(t, err) | ||
|
|
||
| assert.False(t, q.Has("1")) | ||
| }) | ||
| } | ||
|
|
||
| testCase(t, ActionDone) | ||
| testCase(t, ActionRetry) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The review comment is correct in pointing out the absence of test scenarios for an empty queue and concurrent operations in the TestHandleNext function. These scenarios are crucial for a comprehensive test suite to identify potential race conditions or deadlocks. Here are the specific recommendations:
- Empty Queue Scenario: Add a test case to verify the behavior of
handleNextwhen the queue is empty. This will help ensure that the function handles such cases gracefully without errors. - Concurrent Operations: Implement a test case that simulates multiple concurrent operations on the queue. This is important to verify that the queue management is thread-safe and can handle simultaneous accesses without issues.
Analysis chain
In the test TestHandleNext, ensure that the test cases cover scenarios where the queue might be empty or where multiple concurrent operations are performed. This will help in identifying race conditions or deadlocks.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that `TestHandleNext` covers scenarios with empty queue and concurrent operations.
rg --type go $'func TestHandleNext($_) {$$$}'
Length of output: 183
Script:
#!/bin/bash
# Correcting the regex pattern to properly search for the TestHandleNext function in Go files.
rg --type go 'func TestHandleNext'
Length of output: 107
Script:
#!/bin/bash
# Extract the content of the TestHandleNext function to verify the scenarios mentioned.
rg --type go -A 20 'func TestHandleNext' util/persistentqueue/queue_test.go | awk '/func TestHandleNext/,/}/'
Length of output: 589
Script:
#!/bin/bash
# Extract the entire content of the TestHandleNext function to verify the scenarios mentioned.
rg --type go -A 50 -B 2 'func TestHandleNext' util/persistentqueue/queue_test.go
Length of output: 1558
| t.Run("RemoveWait", func(t *testing.T) { | ||
| t.Run("processing of item has not yet started", func(t *testing.T) { | ||
| testCase := func(t *testing.T, action Action) { | ||
| t.Run(fmt.Sprintf("action: %s", action), func(t *testing.T) { | ||
| var q *Queue[*testItem] | ||
| db := newInMemoryBadger(t) | ||
|
|
||
| q = newTestQueueWithDb(db, func(ctx context.Context, item *testItem) (Action, error) { | ||
| return action, nil | ||
| }) | ||
|
|
||
| err := q.Add(&testItem{Id: "1", Timestamp: 1, Data: "data1"}) | ||
| require.NoError(t, err) | ||
|
|
||
| waitCh, err := q.RemoveWait("1") | ||
| require.NoError(t, err) | ||
|
|
||
| // Wait channel should be closed | ||
| <-waitCh | ||
|
|
||
| err = q.handleNext() | ||
| require.ErrorIs(t, err, errRemoved) | ||
|
|
||
| assert.False(t, q.Has("1")) | ||
| }) | ||
| } | ||
|
|
||
| testCase(t, ActionDone) | ||
| testCase(t, ActionRetry) | ||
| }) | ||
| t.Run("processing has started", func(t *testing.T) { | ||
| testCase := func(t *testing.T, action Action) { | ||
| t.Run(fmt.Sprintf("action: %s", action), func(t *testing.T) { | ||
| var q *Queue[*testItem] | ||
| db := newInMemoryBadger(t) | ||
|
|
||
| var waitCh chan struct{} | ||
| q = newTestQueueWithDb(db, func(ctx context.Context, item *testItem) (Action, error) { | ||
| var err error | ||
| waitCh, err = q.RemoveWait(item.Key()) | ||
| require.NoError(t, err) | ||
| return action, nil | ||
| }) | ||
|
|
||
| err := q.Add(&testItem{Id: "1", Timestamp: 1, Data: "data1"}) | ||
| require.NoError(t, err) | ||
|
|
||
| err = q.handleNext() | ||
| require.NoError(t, err) | ||
|
|
||
| // Wait channel should be closed | ||
| <-waitCh | ||
| assert.False(t, q.Has("1")) | ||
|
|
||
| // Remove again, expect that wait channel is closed immediately | ||
| waitCh, err = q.RemoveWait("1") | ||
| require.NoError(t, err) | ||
| <-waitCh | ||
| }) | ||
| } | ||
|
|
||
| testCase(t, ActionDone) | ||
| testCase(t, ActionRetry) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
For the test cases under RemoveWait, consider adding assertions to check the state of the queue after operations to ensure that the queue's integrity is maintained throughout the test.
core/files/reconciler/reconciler.go
Outdated
| func (r *reconciler) Init(a *app.App) error { | ||
| r.objectStore = app.MustComponent[objectstore.ObjectStore](a) | ||
| r.fileSync = app.MustComponent[filesync.FileSync](a) | ||
| r.fileStorage = app.MustComponent[filestorage.FileStorage](a) | ||
| r.objectGetter = app.MustComponent[cache.ObjectGetter](a) | ||
|
|
||
| r.fileSync.OnUploaded(r.markAsReconciled) | ||
|
|
||
| dbProvider := app.MustComponent[datastore.Datastore](a) | ||
| db, err := dbProvider.LocalStorage() | ||
| if err != nil { | ||
| return fmt.Errorf("get badger: %w", err) | ||
| } | ||
|
|
||
| r.isStartedStore = keyvaluestore.New(db, []byte("file_reconciler/is_started"), func(val bool) ([]byte, error) { | ||
| return json.Marshal(val) | ||
| }, func(data []byte) (bool, error) { | ||
| var val bool | ||
| err := json.Unmarshal(data, &val) | ||
| return val, err | ||
| }) | ||
| r.deletedFiles = keyvaluestore.New(db, []byte("file_reconciler/deleted_files"), func(_ struct{}) ([]byte, error) { | ||
| return []byte(""), nil | ||
| }, func(data []byte) (struct{}, error) { | ||
| return struct{}{}, nil | ||
| }) | ||
| r.rebindQueue = persistentqueue.New(persistentqueue.NewBadgerStorage(db, []byte("queue/file_reconciler/rebind"), makeQueueItem), log, r.rebindHandler) | ||
| return nil |
There was a problem hiding this comment.
Initialization in Init method is robust, correctly setting up components and handling potential errors. Consider adding more detailed logging for each step to improve traceability.
+ log.Info("Initializing components for reconciler")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (r *reconciler) Init(a *app.App) error { | |
| r.objectStore = app.MustComponent[objectstore.ObjectStore](a) | |
| r.fileSync = app.MustComponent[filesync.FileSync](a) | |
| r.fileStorage = app.MustComponent[filestorage.FileStorage](a) | |
| r.objectGetter = app.MustComponent[cache.ObjectGetter](a) | |
| r.fileSync.OnUploaded(r.markAsReconciled) | |
| dbProvider := app.MustComponent[datastore.Datastore](a) | |
| db, err := dbProvider.LocalStorage() | |
| if err != nil { | |
| return fmt.Errorf("get badger: %w", err) | |
| } | |
| r.isStartedStore = keyvaluestore.New(db, []byte("file_reconciler/is_started"), func(val bool) ([]byte, error) { | |
| return json.Marshal(val) | |
| }, func(data []byte) (bool, error) { | |
| var val bool | |
| err := json.Unmarshal(data, &val) | |
| return val, err | |
| }) | |
| r.deletedFiles = keyvaluestore.New(db, []byte("file_reconciler/deleted_files"), func(_ struct{}) ([]byte, error) { | |
| return []byte(""), nil | |
| }, func(data []byte) (struct{}, error) { | |
| return struct{}{}, nil | |
| }) | |
| r.rebindQueue = persistentqueue.New(persistentqueue.NewBadgerStorage(db, []byte("queue/file_reconciler/rebind"), makeQueueItem), log, r.rebindHandler) | |
| return nil | |
| func (r *reconciler) Init(a *app.App) error { | |
| log.Info("Initializing components for reconciler") | |
| r.objectStore = app.MustComponent[objectstore.ObjectStore](a) | |
| r.fileSync = app.MustComponent[filesync.FileSync](a) | |
| r.fileStorage = app.MustComponent[filestorage.FileStorage](a) | |
| r.objectGetter = app.MustComponent[cache.ObjectGetter](a) | |
| r.fileSync.OnUploaded(r.markAsReconciled) | |
| dbProvider := app.MustComponent[datastore.Datastore](a) | |
| db, err := dbProvider.LocalStorage() | |
| if err != nil { | |
| return fmt.Errorf("get badger: %w", err) | |
| } | |
| r.isStartedStore = keyvaluestore.New(db, []byte("file_reconciler/is_started"), func(val bool) ([]byte, error) { | |
| return json.Marshal(val) | |
| }, func(data []byte) (bool, error) { | |
| var val bool | |
| err := json.Unmarshal(data, &val) | |
| return val, err | |
| }) | |
| r.deletedFiles = keyvaluestore.New(db, []byte("file_reconciler/deleted_files"), func(_ struct{}) ([]byte, error) { | |
| return []byte(""), nil | |
| }, func(data []byte) (struct{}, error) { | |
| return struct{}{}, nil | |
| }) | |
| r.rebindQueue = persistentqueue.New(persistentqueue.NewBadgerStorage(db, []byte("queue/file_reconciler/rebind"), makeQueueItem), log, r.rebindHandler) | |
| return nil |
| func (r *reconciler) rebindHandler(ctx context.Context, item *queueItem) (persistentqueue.Action, error) { | ||
| err := r.fileSync.CancelDeletion(item.ObjectId, item.FileId) | ||
| if err != nil { | ||
| return persistentqueue.ActionRetry, fmt.Errorf("cancel deletion: %w", err) | ||
| } | ||
|
|
||
| log.Warn("add to queue", zap.String("objectId", item.ObjectId), zap.String("fileId", item.FileId.FileId.String())) | ||
| err = r.fileSync.AddFile(item.ObjectId, item.FileId, false, false) | ||
| if err != nil { | ||
| return persistentqueue.ActionRetry, fmt.Errorf("upload file: %w", err) | ||
| } | ||
|
|
||
| return persistentqueue.ActionDone, nil |
There was a problem hiding this comment.
The rebindHandler method effectively handles the rebind process, including retry logic on failure. Consider adding more detailed logging to trace the rebind actions.
+ log.Debug("Processing rebind for", zap.String("objectId", item.ObjectId), zap.String("fileId", item.FileId.FileId.String()))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (r *reconciler) rebindHandler(ctx context.Context, item *queueItem) (persistentqueue.Action, error) { | |
| err := r.fileSync.CancelDeletion(item.ObjectId, item.FileId) | |
| if err != nil { | |
| return persistentqueue.ActionRetry, fmt.Errorf("cancel deletion: %w", err) | |
| } | |
| log.Warn("add to queue", zap.String("objectId", item.ObjectId), zap.String("fileId", item.FileId.FileId.String())) | |
| err = r.fileSync.AddFile(item.ObjectId, item.FileId, false, false) | |
| if err != nil { | |
| return persistentqueue.ActionRetry, fmt.Errorf("upload file: %w", err) | |
| } | |
| return persistentqueue.ActionDone, nil | |
| func (r *reconciler) rebindHandler(ctx context.Context, item *queueItem) (persistentqueue.Action, error) { | |
| log.Debug("Processing rebind for", zap.String("objectId", item.ObjectId), zap.String("fileId", item.FileId.FileId.String())) | |
| err := r.fileSync.CancelDeletion(item.ObjectId, item.FileId) | |
| if err != nil { | |
| return persistentqueue.ActionRetry, fmt.Errorf("cancel deletion: %w", err) | |
| } | |
| log.Warn("add to queue", zap.String("objectId", item.ObjectId), zap.String("fileId", item.FileId.FileId.String())) | |
| err = r.fileSync.AddFile(item.ObjectId, item.FileId, false, false) | |
| if err != nil { | |
| return persistentqueue.ActionRetry, fmt.Errorf("upload file: %w", err) | |
| } | |
| return persistentqueue.ActionDone, nil |
core/files/fileobject/service.go
Outdated
| @@ -109,6 +111,7 @@ func (s *service) Init(a *app.App) error { | |||
| s.objectStore = app.MustComponent[objectstore.ObjectStore](a) | |||
| s.fileStore = app.MustComponent[filestore.FileStore](a) | |||
| s.spaceIdResolver = app.MustComponent[idresolver.Resolver](a) | |||
| s.fileStorage = app.MustComponent[filestorage.FileStorage](a) | |||
There was a problem hiding this comment.
In the Init method, the initialization of fileStorage is critical. Add error handling for the case where fileStorage cannot be retrieved or initialized correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- .mockery.yaml (2 hunks)
- core/filestorage/filesync/filesync.go (5 hunks)
- core/filestorage/filesync/remove.go (5 hunks)
- core/filestorage/filesync/remove_test.go (1 hunks)
- core/filestorage/rpcstore/mock_rpcstore/mock_RpcStore.go (1 hunks)
- core/filestorage/rpcstore/mock_rpcstore/mock_Service.go (1 hunks)
- core/filestorage/rpcstore/service.go (1 hunks)
- util/persistentqueue/queue.go (6 hunks)
Files skipped from review due to trivial changes (1)
- core/filestorage/rpcstore/service.go
Files skipped from review as they are similar to previous changes (3)
- .mockery.yaml
- core/filestorage/filesync/filesync.go
- util/persistentqueue/queue.go
Additional comments not posted (20)
core/filestorage/filesync/remove_test.go (1)
24-95: The test functionTestCancelDeletionis well-structured and covers the cancellation process of file deletions effectively. The use of mocks and assertions is appropriate, ensuring that the function behaves as expected under different conditions.core/filestorage/filesync/remove.go (7)
13-17: The structdeletionQueueItemis well-defined with appropriate fields for managing deletion tasks. This struct is crucial for the queue management system in handling file deletions.
23-28: The methodValidateindeletionQueueItemcorrectly checks the validity of theFileId. This is a good practice to ensure data integrity before processing the deletion.
Line range hint
33-44: The methodDeleteFileeffectively orchestrates the deletion process, including validation and removal from uploading queues. The error wrapping provides clarity on the error origin, which is beneficial for debugging.
Line range hint
51-66: The methoddeletionHandlerhandles the deletion process within the queue. The detailed logging and error handling ensure that any issues during the deletion process are captured and addressed appropriately.
Line range hint
73-83: The methodretryDeletionHandlerprovides a mechanism to retry deletions that failed initially. This is crucial for ensuring reliability in the file deletion process, especially in cases of transient errors.
Line range hint
109-121: The methodremoveFromDeletionQueuesefficiently manages the removal of tasks from both the primary and retry deletion queues. The error handling and logging are well-implemented, providing clear feedback on the operation's status.
121-152: The methodCancelDeletionis designed to cancel the deletion process for a file. The use ofRemoveWaitand context handling withselectstatements ensures that the cancellation respects ongoing operations and system shutdown signals.core/filestorage/rpcstore/mock_rpcstore/mock_Service.go (3)
25-41: The methodInitin the mockServiceis correctly implemented with parameter handling and return value setup. This allows for flexible testing scenarios by adjusting the behavior of the mock based on the input.
71-87: The methodNameprovides a simple mock implementation that returns a service name. This is useful for identifying the service in logs or error messages during testing.
116-134: The methodNewStorein the mockServicecorrectly creates and returns a new instance ofRpcStore. This is crucial for testing components that depend on theRpcStoreinterface.core/filestorage/rpcstore/mock_rpcstore/mock_RpcStore.go (9)
31-59: The methodAccountInfoin the mockRpcStoreis well-implemented with detailed parameter handling and return value setup. This allows for comprehensive testing of the account information retrieval functionality.
89-105: The methodAddprovides a mock implementation that simulates the addition of blocks to the store. This is crucial for testing the storage capabilities of the RPC store in various scenarios.
136-152: The methodAddToFilecorrectly simulates the process of adding blocks to a specific file in the store. This is essential for testing file-specific operations within the RPC store.
185-201: The methodBindCidsprovides a mock implementation for binding CIDs to a file. This functionality is critical for managing file identities and ensuring data integrity in the store.
234-262: The methodCheckAvailabilityis implemented to simulate the checking of block availability in the store. This is important for testing how the store handles data availability queries.
294-310: The methodDeleteprovides a mock implementation for deleting a CID from the store. This is crucial for testing the deletion capabilities and ensuring that the store correctly handles data removal.
341-364: The methodDeleteFilessimulates the deletion of multiple files from the store. This is essential for testing batch deletion operations and their impact on the store's state.
403-438: The methodFilesInfoprovides a mock implementation for retrieving information about multiple files. This functionality is critical for testing the information retrieval capabilities of the store.
585-601: The methodIterateFilesis implemented to simulate the iteration over files in the store. This is important for testing the store's ability to handle bulk data operations efficiently.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- util/keyvaluestore/store.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- util/keyvaluestore/store.go
deff7
changed the base branch from
main
to
go-3446-sqlite-tree-storage-race-condition-when-updating
deff7
changed the base branch from
go-3446-sqlite-tree-storage-race-condition-when-updating
to
go-3446-sqlite-tree-storage-race-condition-when-updating-new
There was a problem hiding this comment.
Actionable comments posted: 8
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.gopb/commands.pb.gois excluded by!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go
Files selected for processing (11)
- .mockery.yaml (2 hunks)
- core/files/fileobject/service.go (3 hunks)
- core/files/reconciler/reconciler.go (1 hunks)
- core/filestorage/rpcstore/mock_rpcstore/mock_rpcstore.go (1 hunks)
- docs/proto.md (5 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- space/spacecore/storage/sqlitestorage/space_test.go (2 hunks)
- space/spacecore/storage/sqlitestorage/sql.go (3 hunks)
- space/spacecore/storage/sqlitestorage/tree_test.go (2 hunks)
- util/testMock/objectstore_mock.go (1 hunks)
Files skipped from review due to trivial changes (1)
- util/testMock/objectstore_mock.go
Files skipped from review as they are similar to previous changes (5)
- .mockery.yaml
- core/files/fileobject/service.go
- core/filestorage/rpcstore/mock_rpcstore/mock_rpcstore.go
- pb/protos/commands.proto
- pb/protos/service/service.proto
Additional comments not posted (12)
space/spacecore/storage/sqlitestorage/sql.go (3)
64-69: Ensure thedeleteTreestatement correctly handles the deletion logic.The SQL statement inserts a new row or updates an existing row in the
treestable, setting theheadscolumn to NULL. This approach is correct for marking a tree as deleted by nullifying its heads.
70-73: Ensure theupdateTreeDelStatusstatement correctly handles the update logic.The SQL statement inserts a new row or updates an existing row in the
treestable, setting thedeleteStatuscolumn. This approach is correct for updating the delete status of a tree.
74-75: Ensure thetreeDelStatusstatement correctly retrieves the delete status.The SQL statement selects the
deleteStatuscolumn from thetreestable based on the tree ID. This approach is correct for retrieving the delete status of a tree.space/spacecore/storage/sqlitestorage/tree_test.go (5)
124-125: Ensure the test case correctly verifies the error when accessing a deleted tree.The test case correctly verifies that accessing a deleted tree returns the
ErrUnknownTreeIderror.
126-132: Ensure the test case correctly verifies that the heads are nulled after deletion.The test case correctly verifies that the
headscolumn is set to NULL after the tree is deleted.
134-138: Ensure the test case correctly verifies that the tree is marked as deleted.The test case correctly verifies that the
HasTreemethod returnsfalsefor a deleted tree.
140-144: Ensure the test case correctly verifies that the delete status is not changed after deletion.The test case correctly verifies that the delete status remains unchanged after the tree is deleted.
146-150: Ensure the test case correctly verifies that the deleted tree ID is still stored.The test case correctly verifies that the deleted tree ID is still present in the stored IDs.
space/spacecore/storage/sqlitestorage/space_test.go (1)
97-119: Ensure the test case correctly verifies setting and retrieving the delete status of a tree.The test case correctly verifies that the delete status can be set and retrieved for a tree, even if the tree row is absent initially.
core/files/reconciler/reconciler.go (2)
76-103: Initialization inInitmethod is robust, correctly setting up components and handling potential errors. Consider adding more detailed logging for each step to improve traceability.+ log.Info("Initializing components for reconciler")
168-180: TherebindHandlermethod effectively handles the rebind process, including retry logic on failure. Consider adding more detailed logging to trace the rebind actions.+ log.Debug("Processing rebind for", zap.String("objectId", item.ObjectId), zap.String("fileId", item.FileId.FileId.String()))docs/proto.md (1)
1883-1884: LGTM! The additions are consistent with the rest of the table.
| - [Rpc.File.Reconcile](#anytype-Rpc-File-Reconcile) | ||
| - [Rpc.File.Reconcile.Request](#anytype-Rpc-File-Reconcile-Request) | ||
| - [Rpc.File.Reconcile.Response](#anytype-Rpc-File-Reconcile-Response) | ||
| - [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) |
There was a problem hiding this comment.
Correct the indentation for consistency.
- - [Rpc.File.Reconcile](#anytype-Rpc-File-Reconcile)
- - [Rpc.File.Reconcile.Request](#anytype-Rpc-File-Reconcile-Request)
- - [Rpc.File.Reconcile.Response](#anytype-Rpc-File-Reconcile-Response)
- - [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error)
+ - [Rpc.File.Reconcile](#anytype-Rpc-File-Reconcile)
+ - [Rpc.File.Reconcile.Request](#anytype-Rpc-File-Reconcile-Request)
+ - [Rpc.File.Reconcile.Response](#anytype-Rpc-File-Reconcile-Response)
+ - [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - [Rpc.File.Reconcile](#anytype-Rpc-File-Reconcile) | |
| - [Rpc.File.Reconcile.Request](#anytype-Rpc-File-Reconcile-Request) | |
| - [Rpc.File.Reconcile.Response](#anytype-Rpc-File-Reconcile-Response) | |
| - [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) | |
| - [Rpc.File.Reconcile](#anytype-Rpc-File-Reconcile) | |
| - [Rpc.File.Reconcile.Request](#anytype-Rpc-File-Reconcile-Request) | |
| - [Rpc.File.Reconcile.Response](#anytype-Rpc-File-Reconcile-Response) | |
| - [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) |
| func (r *reconciler) Run(ctx context.Context) error { | ||
| isStarted, err := r.isStartedStore.Get(isStartedStoreKey) | ||
| if err != nil && !errors.Is(err, keyvaluestore.ErrNotFound) { | ||
| log.Error("get isStarted", zap.Error(err)) | ||
| } | ||
| r.lock.Lock() | ||
| r.isStarted = isStarted | ||
| r.lock.Unlock() | ||
|
|
||
| r.rebindQueue.Run() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure the Run method correctly retrieves the isStarted state and runs the rebind queue.
The method correctly retrieves the isStarted state from the key-value store and runs the rebind queue. Consider adding logging to trace the state retrieval and queue running.
+ log.Info("Running rebind queue")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (r *reconciler) Run(ctx context.Context) error { | |
| isStarted, err := r.isStartedStore.Get(isStartedStoreKey) | |
| if err != nil && !errors.Is(err, keyvaluestore.ErrNotFound) { | |
| log.Error("get isStarted", zap.Error(err)) | |
| } | |
| r.lock.Lock() | |
| r.isStarted = isStarted | |
| r.lock.Unlock() | |
| r.rebindQueue.Run() | |
| return nil | |
| } | |
| func (r *reconciler) Run(ctx context.Context) error { | |
| isStarted, err := r.isStartedStore.Get(isStartedStoreKey) | |
| if err != nil && !errors.Is(err, keyvaluestore.ErrNotFound) { | |
| log.Error("get isStarted", zap.Error(err)) | |
| } | |
| r.lock.Lock() | |
| r.isStarted = isStarted | |
| r.lock.Unlock() | |
| log.Info("Running rebind queue") | |
| r.rebindQueue.Run() | |
| return nil | |
| } |
| func (r *reconciler) Start(ctx context.Context) error { | ||
| err := r.isStartedStore.Set(isStartedStoreKey, true) | ||
| if err != nil { | ||
| return fmt.Errorf("set isStarted: %w", err) | ||
| } | ||
| r.lock.Lock() | ||
| r.isStarted = true | ||
| r.lock.Unlock() | ||
|
|
||
| return r.reconcileRemoteStorage(ctx) | ||
| } |
There was a problem hiding this comment.
Ensure the Start method correctly sets the isStarted state and initiates remote storage reconciliation.
The method correctly sets the isStarted state in the key-value store and initiates the remote storage reconciliation process. Consider adding logging to trace these actions.
+ log.Info("Starting reconciler and initiating remote storage reconciliation")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (r *reconciler) Start(ctx context.Context) error { | |
| err := r.isStartedStore.Set(isStartedStoreKey, true) | |
| if err != nil { | |
| return fmt.Errorf("set isStarted: %w", err) | |
| } | |
| r.lock.Lock() | |
| r.isStarted = true | |
| r.lock.Unlock() | |
| return r.reconcileRemoteStorage(ctx) | |
| } | |
| func (r *reconciler) Start(ctx context.Context) error { | |
| log.Info("Starting reconciler and initiating remote storage reconciliation") | |
| err := r.isStartedStore.Set(isStartedStoreKey, true) | |
| if (err != nil) { | |
| return fmt.Errorf("set isStarted: %w", err) | |
| } | |
| r.lock.Lock() | |
| r.isStarted = true | |
| r.lock.Unlock() | |
| return r.reconcileRemoteStorage(ctx) | |
| } |
| @@ -1254,6 +1258,7 @@ | |||
| - [Rpc.File.ListOffload.Response.Error.Code](#anytype-Rpc-File-ListOffload-Response-Error-Code) | |||
| - [Rpc.File.NodeUsage.Response.Error.Code](#anytype-Rpc-File-NodeUsage-Response-Error-Code) | |||
| - [Rpc.File.Offload.Response.Error.Code](#anytype-Rpc-File-Offload-Response-Error-Code) | |||
| - [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) | |||
There was a problem hiding this comment.
Correct the indentation for consistency.
- - [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code)
+ - [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) | |
| - [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) |
| <a name="anytype-Rpc-File-Reconcile"></a> | ||
|
|
||
| ### Rpc.File.Reconcile | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-File-Reconcile-Request"></a> | ||
|
|
||
| ### Rpc.File.Reconcile.Request | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-File-Reconcile-Response"></a> | ||
|
|
||
| ### Rpc.File.Reconcile.Response | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | error | [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| <a name="anytype-Rpc-File-Reconcile-Response-Error"></a> | ||
|
|
||
| ### Rpc.File.Reconcile.Response.Error | ||
|
|
||
|
|
||
|
|
||
| | Field | Type | Label | Description | | ||
| | ----- | ---- | ----- | ----------- | | ||
| | code | [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) | | | | ||
| | description | [string](#string) | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Reduce excessive blank lines for better readability.
-<a name="anytype-Rpc-File-Reconcile"></a>
-
-### Rpc.File.Reconcile
-
-
-
-
-
-
-
-
-
-<a name="anytype-Rpc-File-Reconcile-Request"></a>
-
-### Rpc.File.Reconcile.Request
-
-
-
-
-
-
-
-
-<a name="anytype-Rpc-File-Reconcile-Response"></a>
-
-### Rpc.File.Reconcile.Response
-
-
-
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| error | [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) | | |
-
-
-
-
-
-
-
-<a name="anytype-Rpc-File-Reconcile-Response-Error"></a>
-
-### Rpc.File.Reconcile.Response.Error
-
-
-
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| code | [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) | | |
| description | [string](#string) | | |
-
-
-
-
-
-
+<a name="anytype-Rpc-File-Reconcile"></a>
+### Rpc.File.Reconcile
+<a name="anytype-Rpc-File-Reconcile-Request"></a>
+### Rpc.File.Reconcile.Request
+<a name="anytype-Rpc-File-Reconcile-Response"></a>
+### Rpc.File.Reconcile.Response
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| error | [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) | | |
+<a name="anytype-Rpc-File-Reconcile-Response-Error"></a>
+### Rpc.File.Reconcile.Response.Error
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| code | [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) | | |
+| description | [string](#string) | | |Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| <a name="anytype-Rpc-File-Reconcile"></a> | |
| ### Rpc.File.Reconcile | |
| <a name="anytype-Rpc-File-Reconcile-Request"></a> | |
| ### Rpc.File.Reconcile.Request | |
| <a name="anytype-Rpc-File-Reconcile-Response"></a> | |
| ### Rpc.File.Reconcile.Response | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | error | [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) | | | | |
| <a name="anytype-Rpc-File-Reconcile-Response-Error"></a> | |
| ### Rpc.File.Reconcile.Response.Error | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | code | [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) | | | | |
| | description | [string](#string) | | | | |
| <a name="anytype-Rpc-File-Reconcile"></a> | |
| ### Rpc.File.Reconcile | |
| <a name="anytype-Rpc-File-Reconcile-Request"></a> | |
| ### Rpc.File.Reconcile.Request | |
| <a name="anytype-Rpc-File-Reconcile-Response"></a> | |
| ### Rpc.File.Reconcile.Response | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | error | [Rpc.File.Reconcile.Response.Error](#anytype-Rpc-File-Reconcile-Response-Error) | | | | |
| <a name="anytype-Rpc-File-Reconcile-Response-Error"></a> | |
| ### Rpc.File.Reconcile.Response.Error | |
| | Field | Type | Label | Description | | |
| | ----- | ---- | ----- | ----------- | | |
| | code | [Rpc.File.Reconcile.Response.Error.Code](#anytype-Rpc-File-Reconcile-Response-Error-Code) | | | | |
| | description | [string](#string) | | | |
| func (r *reconciler) markAsReconciled(fileObjectId string, fileId domain.FullFileId) error { | ||
| if !r.isRunning() { | ||
| return nil | ||
| } | ||
| return r.deletedFiles.Delete(fileId.FileId.String()) | ||
| } |
There was a problem hiding this comment.
Ensure the markAsReconciled method correctly deletes the file ID from the deleted files store.
The method correctly deletes the file ID from the deleted files store if the reconciler is running. Consider adding logging to trace the deletion action.
+ log.Info("Marking file as reconciled", zap.String("fileId", fileId.FileId.String()))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (r *reconciler) markAsReconciled(fileObjectId string, fileId domain.FullFileId) error { | |
| if !r.isRunning() { | |
| return nil | |
| } | |
| return r.deletedFiles.Delete(fileId.FileId.String()) | |
| } | |
| func (r *reconciler) markAsReconciled(fileObjectId string, fileId domain.FullFileId) error { | |
| if !r.isRunning() { | |
| return nil | |
| } | |
| log.Info("Marking file as reconciled", zap.String("fileId", fileId.FileId.String())) | |
| return r.deletedFiles.Delete(fileId.FileId.String()) | |
| } |
| func (r *reconciler) reconcileRemoteStorage(ctx context.Context) error { | ||
| records, err := r.objectStore.Query(database.Query{ | ||
| Filters: []*model.BlockContentDataviewFilter{ | ||
| { | ||
| RelationKey: bundle.RelationKeyFileId.String(), | ||
| Condition: model.BlockContentDataviewFilter_NotEmpty, | ||
| }, | ||
| { | ||
| RelationKey: bundle.RelationKeyIsDeleted.String(), | ||
| Condition: model.BlockContentDataviewFilter_NotEqual, | ||
| Value: pbtypes.Bool(true), | ||
| }, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("query file objects: %w", err) | ||
| } | ||
|
|
||
| haveIds := map[domain.FileId]struct{}{} | ||
| for _, rec := range records { | ||
| fileId := domain.FileId(pbtypes.GetString(rec.Details, bundle.RelationKeyFileId.String())) | ||
| if fileId.Valid() { | ||
| haveIds[fileId] = struct{}{} | ||
| } | ||
| } | ||
|
|
||
| err = r.fileStorage.IterateFiles(ctx, func(fileId domain.FullFileId) { | ||
| if _, ok := haveIds[fileId.FileId]; !ok { | ||
| log.Warn("file not found in local vault, enqueue deletion", zap.String("fileId", fileId.FileId.String())) | ||
| err := r.fileSync.DeleteFile("", fileId) | ||
| if err != nil { | ||
| log.Error("add to deletion queue", zap.String("fileId", fileId.FileId.String()), zap.Error(err)) | ||
| } | ||
| err = r.deletedFiles.Set(fileId.FileId.String(), struct{}{}) | ||
| if err != nil { | ||
| log.Error("add to deleted files", zap.String("fileId", fileId.FileId.String()), zap.Error(err)) | ||
| } | ||
| } | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("iterate files: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure the reconcileRemoteStorage method correctly reconciles the local storage with the remote storage.
The method correctly queries the object store and iterates through the files in the local storage to reconcile them with the remote storage. Consider adding logging to trace the reconciliation process.
+ log.Info("Reconciling remote storage")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (r *reconciler) reconcileRemoteStorage(ctx context.Context) error { | |
| records, err := r.objectStore.Query(database.Query{ | |
| Filters: []*model.BlockContentDataviewFilter{ | |
| { | |
| RelationKey: bundle.RelationKeyFileId.String(), | |
| Condition: model.BlockContentDataviewFilter_NotEmpty, | |
| }, | |
| { | |
| RelationKey: bundle.RelationKeyIsDeleted.String(), | |
| Condition: model.BlockContentDataviewFilter_NotEqual, | |
| Value: pbtypes.Bool(true), | |
| }, | |
| }, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("query file objects: %w", err) | |
| } | |
| haveIds := map[domain.FileId]struct{}{} | |
| for _, rec := range records { | |
| fileId := domain.FileId(pbtypes.GetString(rec.Details, bundle.RelationKeyFileId.String())) | |
| if fileId.Valid() { | |
| haveIds[fileId] = struct{}{} | |
| } | |
| } | |
| err = r.fileStorage.IterateFiles(ctx, func(fileId domain.FullFileId) { | |
| if _, ok := haveIds[fileId.FileId]; !ok { | |
| log.Warn("file not found in local vault, enqueue deletion", zap.String("fileId", fileId.FileId.String())) | |
| err := r.fileSync.DeleteFile("", fileId) | |
| if err != nil { | |
| log.Error("add to deletion queue", zap.String("fileId", fileId.FileId.String()), zap.Error(err)) | |
| } | |
| err = r.deletedFiles.Set(fileId.FileId.String(), struct{}{}) | |
| if err != nil { | |
| log.Error("add to deleted files", zap.String("fileId", fileId.FileId.String()), zap.Error(err)) | |
| } | |
| } | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("iterate files: %w", err) | |
| } | |
| return nil | |
| } | |
| func (r *reconciler) reconcileRemoteStorage(ctx context.Context) error { | |
| log.Info("Reconciling remote storage") | |
| records, err := r.objectStore.Query(database.Query{ | |
| Filters: []*model.BlockContentDataviewFilter{ | |
| { | |
| RelationKey: bundle.RelationKeyFileId.String(), | |
| Condition: model.BlockContentDataviewFilter_NotEmpty, | |
| }, | |
| { | |
| RelationKey: bundle.RelationKeyIsDeleted.String(), | |
| Condition: model.BlockContentDataviewFilter_NotEqual, | |
| Value: pbtypes.Bool(true), | |
| }, | |
| }, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("query file objects: %w", err) | |
| } | |
| haveIds := map[domain.FileId]struct{}{} | |
| for _, rec := range records { | |
| fileId := domain.FileId(pbtypes.GetString(rec.Details, bundle.RelationKeyFileId.String())) | |
| if fileId.Valid() { | |
| haveIds[fileId] = struct{}{} | |
| } | |
| } | |
| err = r.fileStorage.IterateFiles(ctx, func(fileId domain.FullFileId) { | |
| if _, ok := haveIds[fileId.FileId]; !ok { | |
| log.Warn("file not found in local vault, enqueue deletion", zap.String("fileId", fileId.FileId.String())) | |
| err := r.fileSync.DeleteFile("", fileId) | |
| if err != nil { | |
| log.Error("add to deletion queue", zap.String("fileId", fileId.FileId.String()), zap.Error(err)) | |
| } | |
| err = r.deletedFiles.Set(fileId.FileId.String(), struct{}{}) | |
| if err != nil { | |
| log.Error("add to deleted files", zap.String("fileId", fileId.FileId.String()), zap.Error(err)) | |
| } | |
| } | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("iterate files: %w", err) | |
| } | |
| return nil | |
| } |
| <a name="anytype-Rpc-File-Reconcile-Response-Error-Code"></a> | ||
|
|
||
| ### Rpc.File.Reconcile.Response.Error.Code | ||
|
|
||
|
|
||
| | Name | Number | Description | | ||
| | ---- | ------ | ----------- | | ||
| | NULL | 0 | | | ||
| | UNKNOWN_ERROR | 1 | | | ||
| | BAD_INPUT | 2 | | | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Reduce excessive blank lines for better readability.
-<a name="anytype-Rpc-File-Reconcile-Response-Error-Code"></a>
-
-### Rpc.File.Reconcile.Response.Error.Code
-
-
| Name | Number | Description |
| ---- | ------ | ----------- |
| NULL | 0 | |
| UNKNOWN_ERROR | 1 | |
| BAD_INPUT | 2 | |
-
-
-
-<a name="anytype-Rpc-File-SpaceOffload-Response-Error-Code"></a>
-
-### Rpc.File.SpaceOffload.Response.Error.Code
+<a name="anytype-Rpc-File-Reconcile-Response-Error-Code"></a>
+### Rpc.File.Reconcile.Response.Error.Code
+| Name | Number | Description |
+| ---- | ------ | ----------- |
+| NULL | 0 | |
+| UNKNOWN_ERROR | 1 | |
+| BAD_INPUT | 2 | |
+<a name="anytype-Rpc-File-SpaceOffload-Response-Error-Code"></a>
+### Rpc.File.SpaceOffload.Response.Error.CodeCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| <a name="anytype-Rpc-File-Reconcile-Response-Error-Code"></a> | |
| ### Rpc.File.Reconcile.Response.Error.Code | |
| | Name | Number | Description | | |
| | ---- | ------ | ----------- | | |
| | NULL | 0 | | | |
| | UNKNOWN_ERROR | 1 | | | |
| | BAD_INPUT | 2 | | | |
| <a name="anytype-Rpc-File-Reconcile-Response-Error-Code"></a> | |
| ### Rpc.File.Reconcile.Response.Error.Code | |
| | Name | Number | Description | | |
| | ---- | ------ | ----------- | | |
| | NULL | 0 | | | |
| | UNKNOWN_ERROR | 1 | | | |
| | BAD_INPUT | 2 | | | |
| <a name="anytype-Rpc-File-SpaceOffload-Response-Error-Code"></a> | |
| ### Rpc.File.SpaceOffload.Response.Error.Code |
…o-2827-migration-unbind-user-files # Conflicts: # .mockery.yaml # clientlibrary/service/service.pb.go # core/files/fileobject/service.go # docs/proto.md # pb/commands.pb.go # pb/service/service.pb.go
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
pb/protos/commands.proto (1)
3106-3122: Add comprehensive documentation for theReconcilemessage.It's recommended to add comments explaining the purpose and usage of the
Reconcilemessage and its nested structures. This will improve code readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- .mockery.yaml (2 hunks)
- core/anytype/bootstrap.go (2 hunks)
- core/block/editor/factory.go (3 hunks)
- core/block/editor/smartblock/smartblock.go (4 hunks)
- core/files/fileobject/service.go (3 hunks)
- core/filestorage/fileservice.go (3 hunks)
- core/filestorage/filesync/filesync.go (5 hunks)
- core/filestorage/rpcstore/service.go (1 hunks)
- core/filestorage/rpcstore/store.go (2 hunks)
- core/syncstatus/service.go (1 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- util/persistentqueue/queue.go (6 hunks)
- util/persistentqueue/queue_test.go (1 hunks)
Additional context used
golangci-lint
Additional comments not posted (25)
core/syncstatus/service.go (1)
62-64: Ensure the newly added methods for handling file upload events are correctly integrated and tested.Verification successful
The new shell script results show that the methods
OnUploaded,OnUploadStarted, andOnLimitedare correctly integrated and used in various parts of the codebase, including theservice.go,filesync.go,upload.go, and test files. This confirms that the new file upload event handling methods are properly integrated and tested.
- Integration in
service.go: The methods are called to set up event handlers.- Definition in
filesync.go: The methods are defined and implemented.- Usage in
upload.go: The methods are invoked during the file upload process.- Testing: The methods are used in test files to ensure they work as expected.
This comprehensive integration and testing indicate that the new methods are correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and usage of new file upload event handling methods. # Test: Search for usage of `OnUploaded`, `OnUploadStarted`, and `OnLimited` methods. Expect: Correct integration and usage in the codebase. rg --type python -A 3 -B 3 $'OnUploaded|OnUploadStarted|OnLimited'Length of output: 98
Script:
#!/bin/bash # Description: Verify the integration and usage of new file upload event handling methods. # Test: Search for usage of `OnUploaded`, `OnUploadStarted`, and `OnLimited` methods. Expect: Correct integration and usage in the codebase. rg --type go -A 3 -B 3 $'OnUploaded|OnUploadStarted|OnLimited'Length of output: 18259
core/filestorage/fileservice.go (1)
43-43: The addition of theIterateFilesmethod enhances file handling capabilities. Ensure this method is correctly implemented and does not conflict with existing functionalities.Also applies to: 114-116
.mockery.yaml (1)
59-62: Ensure the configurations for generating mocks for theServiceandRpcStoreinterfaces are correctly set up.Verification successful
The
.mockery.yamlfile does indeed contain configurations for generating mocks for theServiceandRpcStoreinterfaces in therpcstorepackage. This confirms that the configurations are correctly set up.
- The
ServiceandRpcStoreinterfaces are listed under thegithub.com/anyproto/anytype-heart/core/filestorage/rpcstorepackage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the configuration for mock generation in the `.mockery.yaml` file. # Test: Check the `.mockery.yaml` file for correct mock generation configurations. Expect: Correct configurations for `Service` and `RpcStore`. cat .mockery.yaml | grep -A 10 -B 10 'Service|RpcStore'Length of output: 57
Script:
#!/bin/bash # Output the entire contents of the .mockery.yaml file for review cat .mockery.yamlLength of output: 5387
core/filestorage/filesync/filesync.go (6)
34-34: UpdatedStatusCallbackto includefileIdparameter for better traceability.
38-38: TheUploadSynchronouslymethod now correctly takes acontext.Contextparameter, aligning with best practices for handling cancellations and timeouts.
42-42: AddedCancelDeletionmethod to allow cancellation of file deletions, enhancing control over file lifecycle management.
75-75: Refactored queue types to usedeletionQueueIteminstead ofQueueItemfor deletion operations, improving type safety and clarity.Also applies to: 81-81, 82-82, 104-104, 105-105
94-94: Initialization ofrpcStoreanddagServiceusing dependency injection ensures better modularity and testability.Also applies to: 95-95
114-114: Enhanced flexibility by allowing multipleonUploadedcallbacks.core/block/editor/factory.go (2)
24-24: Imported thereconcilerpackage to utilize the file reconciliation functionality.
62-62: AddedfileReconcilertoObjectFactoryand initialized it in theInitmethod, integrating file reconciliation capabilities into the object factory lifecycle.Also applies to: 90-90
core/filestorage/rpcstore/store.go (2)
37-37: AddedIterateFilesmethod to theRpcStoreinterface to support iterating over files, enhancing the interface's capabilities.
74-79: RefactoredIterateFilesto use a write operation, which might be necessary for specific use cases. Ensure this aligns with the intended use case to avoid unnecessary write locks.util/persistentqueue/queue.go (4)
172-172: Improved error handling and logging for queue operations, ensuring better traceability and debugging capabilities.Also applies to: 181-181
288-296: Enhanced synchronization by locking during the check and mark operation, preventing race conditions.
298-309: Refined the removal process by ensuring thread safety with locks and improving error handling.
331-356: UpdatedRemoveWaitto handle synchronization and error handling more robustly, ensuring that operations are safe and errors are properly managed.core/anytype/bootstrap.go (1)
245-245: Registering thereconcilercomponent is crucial for the new file reconciliation functionality.This ensures that the new reconciler component is initialized and available for use throughout the application, aligning with the PR's objectives to enhance file handling capabilities.
util/persistentqueue/queue_test.go (2)
456-480: The test cases for item removal after processing has started are well-implemented.These tests are crucial for ensuring that the queue handles dynamic item removal correctly, which is essential for the stability and reliability of queue operations.
481-545: The tests forRemoveWaitfunction cover both scenarios where processing has not started and has already started.These comprehensive tests ensure that the queue's behavior is predictable and correct under different conditions, which is vital for robust queue management.
core/files/fileobject/service.go (1)
122-122: Initialization offileStorageshould include error handling.This comment is a duplicate of the previous one regarding the initialization of
fileStorage. It's crucial to handle potential errors during the initialization to ensure the stability of the application.pb/protos/service/service.proto (1)
153-153: Ensure the newFileReconcileRPC method aligns with the overall architecture and naming conventions of existing RPC methods.core/block/editor/smartblock/smartblock.go (3)
970-970: Ensure consistent error handling with the new error variable.This change correctly replaces the old error variable with the new standardized one, improving consistency.
1003-1003: Consistent error handling with the new error variable enhances maintainability.This change is part of the broader initiative to standardize error handling across the application.
552-552: Update error handling to use the new error variable.- return ErrIsDeleted + return domain.ErrObjectIsDeletedLikely invalid or redundant comment.
core/files/fileobject/service.go
Outdated
| @@ -79,6 +80,7 @@ type service struct { | |||
| fileService files.Service | |||
| fileSync filesync.FileSync | |||
| fileStore filestore.FileStore | |||
| fileStorage filestorage.FileStorage | |||
There was a problem hiding this comment.
Ensure proper error handling for fileStorage initialization.
The initialization of fileStorage is critical for the functionality of this service. It's important to add error handling for the case where fileStorage cannot be retrieved or initialized correctly. This will prevent runtime panics and provide clearer error messages for troubleshooting.
- s.fileStorage = app.MustComponent[filestorage.FileStorage](a)
+ s.fileStorage, err = app.MustComponent[filestorage.FileStorage](a)
+ if err != nil {
+ return fmt.Errorf("failed to initialize fileStorage: %w", err)
+ }Committable suggestion was skipped due to low confidence.
core/filestorage/filesync/upload.go
Outdated
| zap.Error(err)) | ||
| return err | ||
| func (s *fileSync) runOnUploadedHook(fileObjectId string, fileId domain.FullFileId) error { | ||
| var merr multierror.Error |
There was a problem hiding this comment.
Let's use std errors.Join instead of multierr
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.godocs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (3)
- core/files/reconciler/reconciler.go (1 hunks)
- core/filestorage/filesync/upload.go (8 hunks)
- core/filestorage/rpcstore/client.go (2 hunks)
Additional comments not posted (12)
core/files/reconciler/reconciler.go (10)
70-72: Constructor functionNewis correctly implemented.
74-100: Initialization inInitmethod is robust, correctly setting up components and handling potential errors. Consider adding more detailed logging for each step to improve traceability.+ log.Info("Initializing components for reconciler")
103-114: Ensure theRunmethod correctly retrieves theisStartedstate and runs the rebind queue.+ log.Info("Running rebind queue")
116-126: Ensure theStartmethod correctly sets theisStartedstate and initiates remote storage reconciliation.+ log.Info("Starting reconciler and initiating remote storage reconciliation")
128-132: MethodisRunningcorrectly checks the running state with proper thread safety.
134-148: MethodFileObjectHookcorrectly generates a hook function with appropriate condition checks and error handling.+ log.Debug("File object hook triggered", zap.String("objectId", id.ObjectID))
151-163: MethodneedToRebindcorrectly evaluates conditions for rebinding files.
165-177: TherebindHandlermethod effectively handles the rebind process, including retry logic on failure. Consider adding more detailed logging to trace the rebind actions.+ log.Debug("Processing rebind for", zap.String("objectId", item.ObjectId), zap.String("fileId", item.FileId.FileId.String()))
180-185: Ensure themarkAsReconciledmethod correctly deletes the file ID from the deleted files store.+ log.Info("Marking file as reconciled", zap.String("fileId", fileId.FileId.String()))
187-230: Ensure thereconcileRemoteStoragemethod correctly reconciles the local storage with the remote storage.+ log.Info("Reconciling remote storage")core/filestorage/rpcstore/client.go (1)
108-128: Ensure error handling consistency initerateFiles.- return err + return fmt.Errorf("failed to get pool connection: %w", err)core/filestorage/filesync/upload.go (1)
250-250: The methoduploadFileshould handle the context cancellation scenario explicitly to stop the operation gracefully if the context is cancelled.
…-migration-unbind-user-files # Conflicts: # core/file.go
No description provided.