feat: heartbeat task&peer lookup in proc #4179
Conversation
discord9
changed the title
discord9
requested review from
zhongzc,
waynexia,
MichaelScofield and
a team
as code owners
fengjiachun
changed the title
discord9
force-pushed
the
flow_use_heartbeat
branch
from
40589a1
to
8bd6d3c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4179 +/- ##
==========================================
- Coverage 85.06% 84.75% -0.31%
==========================================
Files 1028 1030 +2
Lines 180477 181069 +592
==========================================
- Hits 153514 153462 -52
- Misses 26963 27607 +644 |
WalkthroughThe recent updates introduce significant enhancements across various modules, focusing on peer lookup mechanisms, error handling, and heartbeat functionalities. Notably, a Changes
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: 6
Outside diff range and nitpick comments (3)
src/common/meta/src/peer.rs (1)
72-77: Add documentation for the PeerLookupService trait.The
PeerLookupServicetrait is crucial for the lookup functionality, but it lacks documentation. Providing a brief description of the trait and its methods would enhance code maintainability and readability.src/common/meta/src/ddl.rs (1)
122-123: Document the purpose of peer_lookup_service in DdlContext.The addition of
peer_lookup_servicetoDdlContextis crucial for enabling peer lookups within DDL operations. It would be beneficial to add a comment explaining how this service is used within the context to aid future developers.src/meta-srv/src/lease.rs (1)
Line range hint
37-54: Refactor to reduce code duplication in peer lookup functions.The functions
lookup_datanode_peerandlookup_flownode_peershare a significant amount of logic. Consider abstracting the common logic into a private helper function to reduce code duplication and improve maintainability.+ private async fn lookup_peer_generic(lease_key: LeaseKey, meta_peer_client: &MetaPeerClientRef, lease_secs: Option<u64>) -> Result<Option<Peer>> { + let lease_filter = lease_secs.map(build_lease_filter); + let lease_key_bytes: Vec<u8> = lease_key.clone().try_into()?; + let Some(kv) = meta_peer_client.get(&lease_key_bytes).await? else { + return Ok(None); + }; + let lease_value: LeaseValue = kv.value.try_into()?; + let is_alive = lease_filter.map(|f| f(&lease_value)).unwrap_or(true); + if is_alive { + Ok(Some(Peer { + id: lease_key.node_id, + addr: lease_value.node_addr, + })) + } else { + Ok(None) + } + } - // Remove duplicated logic from `lookup_datanode_peer` and `lookup_flownode_peer` and replace with calls to `lookup_peer_generic`Also applies to: 80-105
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 (19)
- src/cmd/src/standalone.rs (2 hunks)
- src/common/meta/src/ddl.rs (2 hunks)
- src/common/meta/src/ddl/create_flow/metadata.rs (1 hunks)
- src/common/meta/src/ddl/drop_flow.rs (2 hunks)
- src/common/meta/src/ddl/flow_meta.rs (4 hunks)
- src/common/meta/src/ddl_manager.rs (2 hunks)
- src/common/meta/src/peer.rs (2 hunks)
- src/common/meta/src/test_util.rs (2 hunks)
- src/flow/Cargo.toml (1 hunks)
- src/flow/src/adapter.rs (3 hunks)
- src/flow/src/heartbeat.rs (1 hunks)
- src/flow/src/lib.rs (1 hunks)
- src/meta-srv/src/flow_meta_alloc.rs (1 hunks)
- src/meta-srv/src/lease.rs (6 hunks)
- src/meta-srv/src/lib.rs (1 hunks)
- src/meta-srv/src/metasrv.rs (2 hunks)
- src/meta-srv/src/metasrv/builder.rs (4 hunks)
- src/meta-srv/src/procedure/utils.rs (2 hunks)
- tests-integration/src/standalone.rs (2 hunks)
Files skipped from review due to trivial changes (2)
- src/flow/Cargo.toml
- src/meta-srv/src/lib.rs
Additional comments not posted (23)
src/flow/src/lib.rs (2)
29-29: Approved addition of the heartbeat module.This change aligns with the PR's objectives to enhance heartbeat functionality for flownodes.
35-35: Approved re-export of adapter entities.Re-exporting these entities simplifies access to common functionalities used across the project.
src/meta-srv/src/flow_meta_alloc.rs (1)
24-55: Approved addition of FlowPeerAllocator structure and methods.The new
FlowPeerAllocatorstructure aligns with the PR's goals to manage flow metadata more effectively, particularly in allocating peers for flow partitions.src/common/meta/src/ddl/create_flow/metadata.rs (1)
26-30: Approved changes to allocate_flow_id method.The inclusion of
cluster_idin the flow creation process is a necessary update to support the new functionality of managing flows across different clusters.src/common/meta/src/ddl/flow_meta.rs (2)
63-72: Approved updates to the create method in FlowMetadataAllocator.The method's update to include
cluster_idin peer allocation supports the enhanced distributed architecture of the system.
46-54: Approved addition of the with_peer_allocator method.This method enhances the configurability of the
FlowMetadataAllocator, allowing for dynamic peer allocator selection based on system needs.src/common/meta/src/peer.rs (2)
79-79: Approved the addition of PeerLookupServiceRef.The introduction of
PeerLookupServiceRefas a type alias for anArcof thePeerLookupServicetrait is a good practice for simplifying the codebase and improving readability.
81-101: Skip redundant comment about#[cfg(test)]gate.Previous discussions clarified why a
#[cfg(test)]gate isn't feasible here, referencing StackOverflow for more details.src/common/meta/src/test_util.rs (1)
183-183: Ensure consistency in the instantiation of StandalonePeerLookupService.The instantiation of
StandalonePeerLookupServicewithin the test utility ensures that peer lookup functionality can be tested consistently. This is a positive change, enhancing the testing framework's capabilities.src/meta-srv/src/lease.rs (2)
154-191: Approve the implementation of MetaPeerLookupService.The
MetaPeerLookupServiceimplementation provides a centralized way to perform peer lookups, encapsulating the logic in a clean and reusable manner. This is a well-implemented enhancement to the system's architecture.
Line range hint
37-54: Skip redundant comment about documenting public functions.The functions have been adequately documented following previous suggestions.
Also applies to: 80-105
src/flow/src/heartbeat.rs (1)
38-45: Consider usingOption<Duration>for intervals.Using
Option<Duration>forreport_intervalandretry_intervalwould allow for more flexible configuration, such as disabling automatic retry or reporting by setting them toNone. This approach can lead to cleaner control flow handling in the logic.
[REFACTOR_SUGGESTion]- report_interval: Duration, - retry_interval: Duration, + report_interval: Option<Duration>, + retry_interval: Option<Duration>,src/meta-srv/src/procedure/utils.rs (1)
228-228: Use ofDummyPeerLookupServicein test utility.The addition of
DummyPeerLookupServicetonew_ddl_contextis appropriate for testing environments where actual peer lookup isn't necessary. Ensure that this is well documented to avoid confusion in non-test environments.tests-integration/src/standalone.rs (1)
201-201: Integration ofStandalonePeerLookupServicein standalone builder.The integration of
StandalonePeerLookupServiceinto theDdlManagersetup is crucial for simulating peer interactions in a standalone environment. This is a positive change, enhancing test fidelity.src/meta-srv/src/metasrv/builder.rs (3)
45-45: Import ofFlowPeerAllocatoris well-placed.The import statement for
FlowPeerAllocatoris correctly positioned within the import section, maintaining a clean and organized code structure.
245-264: Implementation ofFlowPeerAllocatorin the builder pattern.The integration of
FlowPeerAllocatorusing the builder pattern is well-implemented. The use of Arc and cloning of the selector context is appropriate for thread safety and shared ownership in a multi-threaded environment.
293-295: Correct instantiation ofMetaPeerLookupService.The instantiation of
MetaPeerLookupServicewithin theDdlManagersetup is correct. It properly utilizes the clonedmeta_peer_client, ensuring that the peer lookup service is appropriately configured.src/meta-srv/src/metasrv.rs (1)
487-491: Proper modification tolookup_datanode_peerfunction call.The update to include the
DATANODE_LEASE_SECSas a parameter in thelookup_datanode_peerfunction call is appropriate. It aligns with the changes in the function definition and ensures that lease time is configurable and correctly passed.src/flow/src/adapter.rs (2)
82-83: Approved the addition ofnode_idandgrpcoptions toFlownodeOptions.These additions are consistent with the PR's objectives to enhance node identification and communication capabilities. Please ensure that these new options are being utilized effectively throughout the system.
501-501: Reminder to implement heartbeat tasks.The TODO comment suggests adding heartbeat tasks in the future. Please ensure this task is tracked appropriately in the project management tools to ensure it's not overlooked.
src/cmd/src/standalone.rs (1)
561-561: Approved the integration ofStandalonePeerLookupServiceincreate_ddl_task_executer.The addition of
StandalonePeerLookupServicealigns with the PR's objectives to enhance peer lookup services. Please ensure that this service is properly integrated and tested throughout the system.src/common/meta/src/ddl_manager.rs (2)
813-813: Approved the addition of imports forDummyPeerLookupServiceandPeer.The newly added imports are crucial for supporting the peer lookup functionality introduced in this PR. It's good to see modular use of dummy services for testing or fallback scenarios.
858-858: Ensure appropriate usage ofDummyPeerLookupService.Initializing
peer_lookup_servicewithDummyPeerLookupServicein production code might not be ideal unless it's strictly for fallback or testing. If this is for production use, consider using a more robust service or ensure that there's a clear switch mechanism to toggle between dummy and actual services based on the environment.Consider implementing a factory pattern or dependency injection for better manageability and testing of different peer lookup services.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/meta-srv/src/lease.rs (1)
Line range hint
36-54: Refactor thelookup_datanode_peerfunction to improve readability and error handling.The function's structure is generally good, but consider handling the potential error from
lease_secs.checked_mul(1000).unwrap_or(u64::MAX)more gracefully to avoid panics in production.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- src/cmd/src/standalone.rs (2 hunks)
- src/common/meta/src/ddl/drop_flow.rs (2 hunks)
- src/common/meta/src/ddl_manager.rs (2 hunks)
- src/common/meta/src/peer.rs (2 hunks)
- src/common/meta/src/test_util.rs (2 hunks)
- src/flow/src/heartbeat.rs (1 hunks)
- src/meta-srv/src/lease.rs (6 hunks)
- src/meta-srv/src/metasrv.rs (2 hunks)
- src/meta-srv/src/procedure/utils.rs (2 hunks)
- tests-integration/src/standalone.rs (2 hunks)
Files skipped from review due to trivial changes (1)
- src/common/meta/src/test_util.rs
Files skipped from review as they are similar to previous changes (8)
- src/cmd/src/standalone.rs
- src/common/meta/src/ddl/drop_flow.rs
- src/common/meta/src/ddl_manager.rs
- src/common/meta/src/peer.rs
- src/flow/src/heartbeat.rs
- src/meta-srv/src/metasrv.rs
- src/meta-srv/src/procedure/utils.rs
- tests-integration/src/standalone.rs
Additional comments not posted (1)
src/meta-srv/src/lease.rs (1)
154-186: Review the implementation ofMetaPeerLookupService.The implementation of
MetaPeerLookupServiceis straightforward and adheres to thePeerLookupServicetrait. However, consider adding error handling documentation and possibly unit tests to ensure the robustness of these critical network operations.
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)
- src/meta-srv/src/lease.rs (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/meta-srv/src/lease.rs
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
added heartbeat&peer lookup, allowing flow to register to metasrc& using flownode in procedure
Peerfor a given node idChecklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
lookup_alive_datanode_peertolookup_datanode_peerand addedlookup_flownode_peerto streamline peer lookup functionalities.