feat: flownode use Inserter to write to database
#4323
Conversation
discord9
requested review from
zhongzc,
waynexia,
MichaelScofield and
a team
as code owners
WalkthroughThe recent changes introduce significant refactoring and new features for handling invocations in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StartCommand
participant CacheRegistry
participant CatalogManager
participant DDLTaskExecutor
participant FlowWorkerManager
participant FrontendInvoker
User->>StartCommand: Initiates StartCommand
StartCommand->>CacheRegistry: Clones cache registry
StartCommand->>CatalogManager: Clones catalog manager
StartCommand->>DDLTaskExecutor: Clones DDL task executor
StartCommand->>FlowWorkerManager: Creates and initializes with invoker
FlowWorkerManager->>FrontendInvoker: Uses FrontendInvoker for setup
StartCommand->>User: Returns initialized components
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (8)
- src/cmd/Cargo.toml (2 hunks)
- src/cmd/src/flownode.rs (4 hunks)
- src/cmd/src/standalone.rs (5 hunks)
- src/flow/Cargo.toml (1 hunks)
- src/flow/src/error.rs (2 hunks)
- src/flow/src/lib.rs (1 hunks)
- src/flow/src/server.rs (4 hunks)
- src/meta-client/src/client.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- src/meta-client/src/client.rs
Additional comments not posted (18)
src/flow/src/lib.rs (1)
41-44: LGTM! Verify consistency of exported entities.The added exports align with the new functionality introduced in the PR. Ensure that the names are consistently used across the codebase and correctly spelled.
src/flow/Cargo.toml (1)
50-51: LGTM! Verify necessity of added dependencies.The added dependencies for
operatorandpartitionalign with the new functionality introduced in the PR. Ensure that these dependencies are necessary and used in the codebase.src/cmd/Cargo.toml (1)
Line range hint
19-32:
LGTM! Verify necessity of added dependencies.The added dependencies for
apiandcommon-frontendalign with the new functionality introduced in the PR. Ensure that these dependencies are necessary and used in the codebase.src/flow/src/error.rs (2)
193-199: LGTM! Verify necessity of added error variant.The added error variant
CacheRequiredaligns with the new functionality introduced in the PR. Ensure that this error variant is necessary and used in the codebase.
226-226: LGTM! Verify necessity of added status code handling.The added status code handling for
CacheRequiredaligns with the new functionality introduced in the PR. Ensure that this status code handling is necessary and used in the codebase.src/cmd/src/flownode.rs (5)
29-29: New imports approved.The newly added imports
build_frontend_invoker,FlownodeBuilder, andFlownodeInstanceare necessary for the subsequent changes and are correctly integrated.
38-38: New error variant import approved.The newly added error variant
StartFlownodeSnafuis necessary for the subsequent changes and is correctly integrated.
284-285: Creation and initialization oftable_metadata_managerapproved.The
table_metadata_manageris correctly created and initialized, ensuring proper metadata management.
309-309: Addition ofcatalog_managerinFlownodeBuilderapproved.The
catalog_manageris correctly cloned and passed toFlownodeBuilder, ensuring proper catalog management.
315-325: Invoker creation and setting approved.The
build_frontend_invokermethod is correctly called to create an invoker, which is then set usingset_frontend_invoker. Error handling is appropriately managed withStartFlownodeSnafu.Ensure that all function calls to
build_frontend_invokerandset_frontend_invokerare correctly handled in the codebase.src/flow/src/server.rs (4)
20-51: New imports approved.The newly added imports are necessary for the subsequent changes and are correctly integrated.
327-331: Definition ofRemoteFrondendInvokerstruct approved.The
RemoteFrondendInvokerstruct is correctly defined with necessary fields forinserter,deleter, andstatement_executor.
333-372: Implementation ofRemoteFrondendInvokermethods approved.The
RemoteFrondendInvokermethodsnew,row_inserts, androw_deletesare correctly implemented and handle errors appropriately.
374-438: Definition ofbuild_frontend_invokerfunction approved.The
build_frontend_invokerfunction is correctly defined with necessary parameters forcatalog_manager,kv_backend,layered_cache_registry, andprocedure_executor.src/cmd/src/standalone.rs (4)
47-47: New imports approved.The newly added imports
build_frontend_invokerandFlownodeBuilderare necessary for the subsequent changes and are correctly integrated.
69-69: New error variant import approved.The newly added error variant
StartFlownodeSnafuis necessary for the subsequent changes and is correctly integrated.
451-455: Instantiation and initialization ofdatanodeapproved.The
datanodeis correctly instantiated and initialized usingDatanodeBuilder.
526-536: Invoker creation and setting approved.The
build_frontend_invokermethod is correctly called to create an invoker, which is then set usingset_frontend_invoker. Error handling is appropriately managed withStartFlownodeSnafu.Ensure that all function calls to
build_frontend_invokerandset_frontend_invokerare correctly handled in the codebase.
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 (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (7)
- config/config.md (1 hunks)
- config/flownode.example.toml (1 hunks)
- src/cmd/src/flownode.rs (6 hunks)
- src/cmd/src/standalone.rs (5 hunks)
- src/flow/src/adapter.rs (2 hunks)
- src/flow/src/lib.rs (1 hunks)
- src/flow/src/server.rs (4 hunks)
Files skipped from review due to trivial changes (2)
- config/flownode.example.toml
- src/flow/src/adapter.rs
Files skipped from review as they are similar to previous changes (3)
- src/cmd/src/standalone.rs
- src/flow/src/lib.rs
- src/flow/src/server.rs
Additional comments not posted (5)
src/cmd/src/flownode.rs (4)
29-29: LGTM! Import statements are correctly added.The new imports are necessary for the new functionality introduced in the file.
38-38: LGTM! Error handling is correctly updated.The new error variant
StartFlownodeSnafuis necessary for handling errors during the start of the flownode.
276-277: LGTM!table_metadata_managerinitialization is correctly added.The initialization of
table_metadata_manageris necessary for managing table metadata.
307-317: LGTM! Invoker setup is correctly added.The setup of
RemoteFrondendInvokeris necessary for the new functionality.However, ensure that the
RemoteFrondendInvokeris properly tested and integrated with the rest of the system.config/config.md (1)
Line range hint
1-1:
LGTM! Configuration changes are correctly updated.The removal of
frontend_addris in line with the changes introduced in the PR.However, ensure that all references to
frontend_addrare removed from the documentation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4323 +/- ##
==========================================
- Coverage 85.17% 84.89% -0.28%
==========================================
Files 1061 1060 -1
Lines 188392 188643 +251
==========================================
- Hits 160469 160157 -312
- Misses 27923 28486 +563 |
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 (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (10)
- src/cmd/src/flownode.rs (6 hunks)
- src/cmd/src/standalone.rs (9 hunks)
- src/common/frontend/src/lib.rs (1 hunks)
- src/flow/Cargo.toml (2 hunks)
- src/flow/src/adapter.rs (10 hunks)
- src/flow/src/lib.rs (2 hunks)
- src/flow/src/server.rs (7 hunks)
- src/frontend/src/instance.rs (2 hunks)
- tests-integration/src/standalone.rs (3 hunks)
- tests/runner/src/env.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- src/common/frontend/src/lib.rs
- tests/runner/src/env.rs
Files skipped from review as they are similar to previous changes (4)
- src/cmd/src/standalone.rs
- src/flow/Cargo.toml
- src/flow/src/lib.rs
- src/flow/src/server.rs
Additional comments not posted (12)
tests-integration/src/standalone.rs (3)
19-19: LGTM!The addition of
StartFlownodeSnafufor error handling is appropriate.
212-215: LGTM!The instantiation of
instancewith additional parameters (cache_registry,catalog_manager,ddl_task_executor) is appropriate and aligns with the newFrontendInvokersetup.
222-235: LGTM!The construction of
invokerand its usage inflow_worker_manageris a necessary addition for the new mechanism whereFrontendInvokerhandles frontend interactions.src/cmd/src/flownode.rs (3)
29-29: LGTM!The new imports related to
FlownodeBuilder,FlownodeInstance, andFrontendInvokerare necessary for the new functionality.
38-38: LGTM!The addition of
StartFlownodeSnafufor error handling is appropriate.
Line range hint
276-318:
LGTM!The instantiation of
table_metadata_manager,flownode_builder, and the construction ofinvokerare necessary for the new mechanism whereFrontendInvokerhandles frontend interactions.src/frontend/src/instance.rs (3)
Line range hint
68-68:
LGTM!The removal of the
FrontendInvokerimport aligns with the new mechanism.
Line range hint
68-68:
LGTM!The removal of the
FrontendInvokerimplementation forInstanceis appropriate given the new mechanism.
Line range hint
68-68:
LGTM!The removal of the
row_insertsandrow_deletesmethods is appropriate given the new mechanism.src/flow/src/adapter.rs (3)
44-45: LGTM!The new imports related to
tokio::sync::broadcastandtokio::sync::RwLockare necessary for the new functionality.
122-125: LGTM!The addition of
frontend_invokerto theFlowWorkerManagerstruct is necessary for the new mechanism whereFrontendInvokerhandles frontend interactions.
137-137: LGTM!The new method
set_frontend_invokeris crucial for setting theFrontendInvokerin the new mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
config/config.md (1)
Line range hint
4-4:
Add documentation forInserterandDeleterusage in flownode.The PR summary indicates changes to enable flownode to use
InserterandDeleterfor database operations. Ensure the documentation reflects these changes.Do you want me to generate the documentation or open a GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (14)
- config/config.md (1 hunks)
- config/flownode.example.toml (1 hunks)
- src/cmd/src/flownode.rs (6 hunks)
- src/cmd/src/standalone.rs (9 hunks)
- src/common/frontend/src/lib.rs (1 hunks)
- src/flow/Cargo.toml (2 hunks)
- src/flow/src/adapter.rs (10 hunks)
- src/flow/src/error.rs (2 hunks)
- src/flow/src/lib.rs (2 hunks)
- src/flow/src/server.rs (7 hunks)
- src/frontend/src/instance.rs (2 hunks)
- src/meta-client/src/client.rs (1 hunks)
- tests-integration/src/standalone.rs (3 hunks)
- tests/runner/src/env.rs (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- src/common/frontend/src/lib.rs
- src/flow/Cargo.toml
- src/flow/src/adapter.rs
- src/flow/src/error.rs
- src/flow/src/lib.rs
- tests-integration/src/standalone.rs
- tests/runner/src/env.rs
Additional comments not posted (15)
src/cmd/src/flownode.rs (4)
29-29: Import statement forFrontendInvokerlooks good.The import is correctly added.
276-278: Initialization ofTableMetadataManagerlooks good.The initialization is correctly implemented.
301-318: Building and setting upFrontendInvokerlooks good.The code correctly builds and sets up
FrontendInvokerwith necessary components.Ensure that
FrontendInvokeris correctly integrated and utilized in the rest of the codebase.
307-318: SettingFrontendInvokertoflow_worker_managerlooks good.The code correctly sets
FrontendInvokertoflow_worker_manager.Ensure proper error handling and testing for this integration.
src/flow/src/server.rs (4)
20-21: Import statements forRowDeleteRequestsandRowInsertRequestslook good.The import statements are correctly added.
276-284: BuildingFlowWorkerManagerlooks good.The code correctly builds
FlowWorkerManager.Ensure proper error handling and testing for this integration.
Line range hint
307-318: SettingFrontendInvokertoflow_worker_managerlooks good.The code correctly sets
FrontendInvokertoflow_worker_manager.Ensure proper error handling and testing for this integration.
329-432: Implementation ofFrontendInvokerlooks good.The implementation of
FrontendInvokerand its methods is comprehensive and appears correct.Ensure thorough testing and proper error handling for this implementation.
src/frontend/src/instance.rs (1)
Line range hint
329-432: Implementation ofFrontendInvokerlooks good.The implementation of
FrontendInvokerand its methods is comprehensive and appears correct.Ensure thorough testing and proper error handling for this implementation.
src/meta-client/src/client.rs (1)
108-108: Approve the addition ofenable_procedureto flownode default options.The change ensures that the flownode has procedures enabled by default, aligning with the PR objectives.
src/cmd/src/standalone.rs (5)
47-47: Approve the addition ofFrontendInvokerimport.The import is necessary for the changes introduced in the PR.
70-72: Approve the addition ofStartFlownodeSnafuerror variant.The error variant is necessary for handling errors related to starting the flownode.
219-220: Approve the addition offlow_worker_managerfield and TODO comment.The new field is necessary for managing flow workers in the instance, and the TODO comment indicates future work.
253-255: Approve the addition of code to start the flow worker manager.The added code ensures that the flow worker manager runs in the background when the instance starts.
274-282: Approve the addition of code to send a shutdown signal to the flow worker manager.The added code ensures that the flow worker manager is properly shut down when the instance stops.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/cmd/src/flownode.rs (6 hunks)
- src/cmd/src/standalone.rs (9 hunks)
- src/flow/src/adapter.rs (10 hunks)
- tests-integration/src/standalone.rs (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/flow/src/adapter.rs
- tests-integration/src/standalone.rs
Additional comments not posted (8)
src/cmd/src/flownode.rs (4)
29-29: Imports look good.The added imports
FlownodeBuilder,FlownodeInstance, andFrontendInvokerare necessary for the new functionality.
38-38: Error handling update is appropriate.The inclusion of
StartFlownodeSnafuin error handling is consistent with the new functionality.
276-277: Instantiation and initialization oftable_metadata_manageris correct.The code correctly sets up and initializes the
table_metadata_managerwith proper error handling.
307-318: Instantiation and setting ofFrontendInvokeris correct.The code correctly instantiates
FrontendInvokerusingbuild_fromand sets it for theflow_worker_manager.src/cmd/src/standalone.rs (4)
47-47: Imports look good.The added imports
FlowWorkerManager,FlownodeBuilder, andFrontendInvokerare necessary for the new functionality.
70-72: Error handling update is appropriate.The inclusion of
StartFlownodeSnafuandShutdownFlownodeSnafuin error handling is consistent with the new functionality.
219-221: Instantiation offlow_worker_managerandflow_shutdownis correct.The code correctly sets up and initializes the
flow_worker_managerandflow_shutdownin theInstancestruct.
542-552: Instantiation and setting ofFrontendInvokeris correct.The code correctly instantiates
FrontendInvokerusingbuild_fromand sets it for theflow_worker_manager.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/cmd/src/flownode.rs (6 hunks)
- src/cmd/src/standalone.rs (9 hunks)
- src/flow/src/adapter.rs (10 hunks)
- src/flow/src/server.rs (7 hunks)
- tests-integration/src/standalone.rs (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/cmd/src/flownode.rs
- src/cmd/src/standalone.rs
- src/flow/src/adapter.rs
- src/flow/src/server.rs
- tests-integration/src/standalone.rs
I hereby agree to the terms of the GreptimeDB CLA.
flownode now use
Inserter/Deleterto write to database, hence removing need to specify frontend addr and suchRemoteFrontendInvokerand it's builder to build a frontend invokerChecklist
Summary by CodeRabbit
New Features
build_frontend_invoker.Refactor
Bug Fixes
FlowWorkerManagerto handle shutdown signals more effectively.Documentation