refactor(katana): separate node service task #2413
Conversation
WalkthroughOhayo, sensei! The pull request introduces several structural changes to the Katana framework, including the addition of the Changes
Possibly related PRs
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- crates/katana/core/src/service/messaging/mod.rs (2 hunks)
- crates/katana/core/src/service/metrics.rs (1 hunks)
- crates/katana/core/src/service/mod.rs (2 hunks)
- crates/katana/node/src/lib.rs (3 hunks)
- crates/katana/tasks/src/manager.rs (1 hunks)
- crates/katana/tasks/src/task.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- crates/katana/core/src/service/metrics.rs
- crates/katana/tasks/src/manager.rs
Additional comments not posted (12)
crates/katana/core/src/service/mod.rs (3)
34-34: LGTM sensei!The renaming of the
NodeServicestruct toBlockProductionTaskand the change in themetricsfield type fromServiceMetricstoBlockProducerMetricsalign well with the refactoring goal of separating the block production task from the messaging task.Also applies to: 42-42
45-52: Looks good sensei!The removal of the
messagingfield initialization from thenewconstructor aligns with the refactoring goal of separating the block production task from the messaging task.
Line range hint
55-91: Ohayo sensei! The changes look great.The removal of the
messagingrelated polling logic and the updates to the metrics and error logging in theFuturetrait implementation for theBlockProductionTaskstruct align well with the refactoring goal of separating the block production task from the messaging task.crates/katana/tasks/src/task.rs (1)
136-136: Ohayo, sensei! The change in error handling logic looks good to me.The modification ensures that the task is cancelled immediately when a panic occurs, before logging the error. This prioritizes task cancellation and may affect the relative timing of cancellation and error reporting. However, the overall control flow remains intact.
crates/katana/core/src/service/messaging/mod.rs (4)
40-43: Ohayo sensei! The imports look good to me.The imports are necessary for implementing the
Futuretrait forMessagingTask.
50-51: Ohayo sensei! The imports are spot-on.The
StreamExttrait is likely used for working with asynchronous streams in theMessagingTaskimplementation, and theExecutorFactorytrait is used as a generic parameter for theMessagingTaskstruct.
210-221: Ohayo sensei! TheMessagingTaskstruct looks good.The struct is parameterized by a generic type
EFthat must implement theExecutorFactorytrait, and it has a constructor that takes aMessagingService<EF>as an argument. This struct is likely used to represent an asynchronous task for handling messaging operations using theMessagingService.
223-243: Ohayo sensei! TheFutureimplementation forMessagingTasklooks solid.The
pollmethod continuously polls theMessagingServicefor outcomes using awhile letloop, handling two specific cases ofMessagingOutcome:GatherandSend. It logs the number of messages collected or sent, respectively. If no messages are ready, the method returnsPoll::Pending.This implementation allows
MessagingTaskto be used in asynchronous contexts, continuously polling theMessagingServicefor outcomes and handling them accordingly.crates/katana/node/src/lib.rs (4)
22-23: Ohayo sensei! The new imports look good to me.The imports are necessary for the subsequent changes in the file.
Line range hint
176-186: Ohayo sensei! The block producer setup looks good to me.The code segment correctly builds the block producer based on the sequencer configuration.
213-224: Ohayo sensei! The messaging task setup looks good to me.The code segment correctly creates a
TaskManagerand builds and spawns the messaging task if the messaging configuration is provided.
228-231: Ohayo sensei! The block production task setup looks good to me.The code segment correctly builds and spawns the block production task.
| // TODO: remove the messaging feature flag | ||
| // TODO: move the tasks to a separate module |
There was a problem hiding this comment.
Ohayo sensei! Noted the TODO comments.
The TODO comments indicate that the messaging feature flag should be removed and the tasks should be moved to a separate module.
Do you want me to open a GitHub issue to track these refactoring tasks?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2413 +/- ##
==========================================
- Coverage 67.94% 67.92% -0.03%
==========================================
Files 364 364
Lines 47770 47760 -10
==========================================
- Hits 32459 32440 -19
- Misses 15311 15320 +9 ☔ View full report in Codecov by Sentry. |
small refactor/optimisation separating the
NodeServicetask which currently encapsulates both the messaging and block production task.this PR separate it into two different tasks. easier to reason about the scope of each task and allowing them to be scheduled by
Tokioon different worker threads (parallelized)Summary by CodeRabbit
MessagingTaskfor improved asynchronous messaging handling.BlockProductionTaskto streamline block production processes.CriticalTaskBuilderto prioritize task cancellation on panic.shutdownmethod inTaskManagerfor clarity.ServiceMetricsand focusing onBlockProducerMetrics.