GO-3453: progress bars protocol #1274
Conversation
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
WalkthroughThe changes primarily involve refactoring how progress messages are handled across various processes such as file downloads, imports, and exports. The Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant Server
participant ProcessService
Client->>Server: ProcessSubscribe(Request)
Server->>ProcessService: Subscribe(token)
ProcessService->>Server: Subscribe(Response)
Server->>Client: ProcessSubscribe(Response)
Client->>Server: ProcessUnsubscribe(Request)
Server->>ProcessService: Unsubscribe(token)
ProcessService->>Server: Unsubscribe(Response)
Server->>Client: ProcessUnsubscribe(Response)
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: 5
Outside diff range and nitpick comments (3)
core/block/process/queue.go (1)
189-189: The update to theInfomethod to use the newMessagefield aligns with the refactoring of process message handling. Consider adding a comment to clarify the purpose and expected content of theMessagefield for future maintainers.core/block/editor/file/file.go (1)
382-382: The update to theInfomethod to use the newMessagefield aligns with the refactoring of process message handling. Consider adding a comment to clarify the purpose and expected content of theMessagefield for future maintainers.pb/protos/service/service.proto (1)
295-296: Ensure the new RPC methodsProcessSubscribeandProcessUnsubscribeare properly documented.Adding comments to describe the purpose and usage of these new RPC methods would improve maintainability and clarity for other developers.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (6)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gocore/event/mock_event/mock_Sender.gois excluded by!**/mock_*.godocs/proto.mdis excluded by!**/proto.mdpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/events.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (20)
- core/block/editor/file/file.go (1 hunks)
- core/block/export/export.go (1 hunks)
- core/block/files.go (1 hunks)
- core/block/import/csv/converter_test.go (18 hunks)
- core/block/import/html/converter_test.go (1 hunks)
- core/block/import/importer.go (2 hunks)
- core/block/import/notion/api/search/search_test.go (1 hunks)
- core/block/import/pb/converter_test.go (7 hunks)
- core/block/import/txt/converter_test.go (1 hunks)
- core/block/process/progress.go (3 hunks)
- core/block/process/queue.go (1 hunks)
- core/block/process/service.go (4 hunks)
- core/block/process/service_test.go (1 hunks)
- core/event/event.go (2 hunks)
- core/event/event_grpc.go (2 hunks)
- core/process.go (2 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/events.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- util/builtinobjects/builtinobjects.go (1 hunks)
Additional comments not posted (22)
core/event/event.go (1)
16-17: The addition ofBroadcastExceptSessionsto theSenderinterface enhances the flexibility of event broadcasting by allowing exceptions. Good design choice.core/block/import/txt/converter_test.go (1)
20-20: Updated progress initialization to use&pb.ModelProcessMessageOfImport{}aligns with the new protocol for process messages. This change enhances flexibility and maintainability.core/process.go (2)
30-42: The implementation ofProcessSubscribeis robust, handling context initialization, subscription logic, and error management effectively.
44-56: The implementation ofProcessUnsubscribeeffectively manages the unsubscribe logic with proper error handling and context management.core/block/files.go (1)
29-29: Updated progress initialization to&pb.ModelProcessMessageOfSaveFile{}is consistent with the new protocol for process messages, enhancing flexibility and maintainability.core/block/process/service_test.go (2)
62-86: TheTestService_Subscribefunction effectively tests the subscription logic, ensuring that tokens are managed correctly in thedisabledProcessEventmap.
88-101: TheTestService_Unsubscribefunction effectively tests the unsubscribe logic, ensuring that tokens are correctly added to thedisabledProcessEventmap.core/block/process/progress.go (1)
25-30: Refactoring of theNewProgressfunction and theprogressstruct to use theprocessMessagefield aligns with the new protocol for process messages, enhancing flexibility and maintainability.Also applies to: 39-41, 137-137
core/block/process/service.go (1)
Line range hint
101-111: The update to useBroadcastExceptSessionsin themonitormethod enhances the functionality by allowing selective broadcasting. Ensure that thegetExcludedSessionsmethod is thoroughly tested to handle concurrent modifications safely.core/block/import/html/converter_test.go (1)
32-32: The update to useModelProcessMessageOfImportin the test setup is consistent with the new progress message handling. Consider adding more test cases to cover different scenarios and error conditions.core/block/import/pb/converter_test.go (1)
46-46: The update to useModelProcessMessageOfImportin the test setup is consistent with the new progress message handling. Consider adding more test cases to cover different scenarios and error conditions.Also applies to: 66-66, 93-93, 111-111, 124-124, 144-144, 180-180
core/block/import/importer.go (2)
244-246: The refactoring ofsetupProgressBarto usepb.IsModelProcessMessagealigns with the overall PR objectives to enhance flexibility in process message handling.
281-281: The update inImportWebto use&pb.ModelProcessMessageOfImport{}for progress initialization is consistent with the refactoring strategy outlined in the PR.core/block/import/notion/api/search/search_test.go (1)
35-35: The update inTest_GetDatabaseSuccessto use&pb.ModelProcessMessageOfImport{}for progress initialization is consistent with the refactoring strategy outlined in the PR.util/builtinobjects/builtinobjects.go (1)
539-539: LGTM! The update to use&pb.ModelProcessMessageOfImport{}insetupProgressaligns with the refactoring of process messages.core/block/import/csv/converter_test.go (1)
25-25: LGTM! The updates to use&pb.ModelProcessMessageOfImport{}in various test functions align with the refactoring of process messages.Also applies to: 50-50, 83-83, 112-112, 153-153, 172-172, 212-212, 240-240, 287-287, 342-342, 363-363, 384-384, 412-412, 458-458, 564-564, 606-606, 674-674, 712-712
core/block/export/export.go (5)
102-104: Update to use newpb.ModelProcessMessageOfExportfor process message initialization.This change aligns with the PR's objective to refactor process message handling using the new protocol buffer definitions. It enhances type safety and clarity in the code.
102-104: Refactored process message initialization inExportmethod.The refactoring to use
pb.ModelProcessMessageOfExportinstead of a generic type or enum improves the clarity and type safety of the process message handling. This change is consistent with the overall PR objectives of enhancing the process message protocol.
102-104: Concurrency handling inexportDocs.The use of atomic operations for incrementing the success count and the structured handling of concurrent tasks ensure that the export process is both efficient and thread-safe. This is a good practice in handling concurrency in Go.
102-104: Improved error handling insendNotification.The method now appropriately determines the type of notification to send based on the presence of an error, which enhances the robustness and user-friendliness of the export functionality.
102-104: Robust file cleanup incleanupFile.The method ensures that files are properly closed and deleted if necessary, which prevents resource leakage and maintains the integrity of the file system during export operations.
pb/protos/events.proto (1)
1079-1093: The addition of specific message types for process actions enhances the granularity and specificity of process handling.
| message Unsubscribe { | ||
| message Request {} | ||
|
|
There was a problem hiding this comment.
Add documentation for the Unsubscribe method.
Similar to the Subscribe method, the Unsubscribe method is missing documentation. Adding detailed comments will help developers understand the purpose and usage of this method more clearly.
| message Subscribe { | ||
| message Request {} | ||
|
|
||
| message Response { | ||
| Error error = 1; | ||
|
|
||
| message Error { | ||
| Code code = 1; | ||
| string description = 2; | ||
|
|
||
| enum Code { | ||
| NULL = 0; | ||
| UNKNOWN_ERROR = 1; | ||
| BAD_INPUT = 2; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add documentation for the Subscribe method.
The Subscribe method lacks documentation. It's important to maintain comprehensive documentation, especially for public APIs, to ensure they are easily understandable and usable by developers. Consider adding a brief description of what the method does, its parameters, and what it returns.
| // BroadcastExceptSessions broadcasts the event to session except provided | ||
| func (es *GrpcSender) BroadcastExceptSessions(event *pb.Event, exceptTokens []string) { | ||
| es.ServerMutex.RLock() | ||
| defer es.ServerMutex.RUnlock() | ||
|
|
||
| for _, s := range es.Servers { | ||
| if !slices.Contains(exceptTokens, s.Token) { | ||
| es.sendEvent(s, event) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of BroadcastExceptSessions method looks correct and thread-safe.
Would you like me to help create a unit test for this method to ensure it behaves as expected under various scenarios?
| // Subscribe remove session from the map of disabled sessions | ||
| Subscribe(token string) | ||
| // Unsubscribe add session to the map of disabled sessions | ||
| Unsubscribe(token string) |
There was a problem hiding this comment.
The implementation of Subscribe and Unsubscribe methods along with the getExcludedSessions helper function is correct. However, consider adding more robust synchronization or using concurrent-safe data structures to manage the disabledProcessEvent map to prevent potential race conditions.
- map[string]struct{}
+ sync.MapAlso applies to: 179-199
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.
| // Subscribe remove session from the map of disabled sessions | |
| Subscribe(token string) | |
| // Unsubscribe add session to the map of disabled sessions | |
| Unsubscribe(token string) | |
| // Subscribe remove session from the map of disabled sessions | |
| Subscribe(token string) | |
| // Unsubscribe add session to the map of disabled sessions | |
| Unsubscribe(token string) |
| func (es *CallbackSender) BroadcastExceptSessions(event *pb.Event, exceptTokens []string) { | ||
| es.callback(event) | ||
| } |
There was a problem hiding this comment.
The implementation of BroadcastExceptSessions should exclude the specified sessions. Currently, it broadcasts to all sessions.
- es.callback(event)
+ for _, session := range es.sessions {
+ if !contains(exceptTokens, session.token) {
+ es.callback(event)
+ }
+ }Please ensure that sessions and contains logic is implemented to handle the exclusion properly.
Committable suggestion was skipped due low confidence.
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
Signed-off-by: AnastasiaShemyakinskaya <shem98a@mail.ru>
https://linear.app/anytype/issue/GO-3453/[next]-progress-bars-protocol