-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: forgot collect flownode clusterinfo handler #4236
feat: forgot collect flownode clusterinfo handler #4236
Conversation
WalkthroughThe meta-handler updates introduced Changes
Sequence DiagramsBelow is a sequence diagram demonstrating the updated heartbeat handling flow with the new sequenceDiagram
participant Client
participant ExtractStatHandler
participant CollectStatsHandler
participant DataStore
Client->>+ExtractStatHandler: Send Heartbeat Request
ExtractStatHandler->>ExtractStatHandler: Validate Role
ExtractStatHandler->>+CollectStatsHandler: Forward Heartbeat Request
CollectStatsHandler->>CollectStatsHandler: Process Statistics
CollectStatsHandler->>DashMap: Update Cache
CollectStatsHandler->>DataStore: Persist Stats with PutRequest
CollectStatsHandler-->>-ExtractStatHandler: Acknowledge Receipt
ExtractStatHandler-->>-Client: Heartbeat Acknowledged
Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/meta-srv/src/handler.rs (3 hunks)
- src/meta-srv/src/handler/collect_stats_handler.rs (2 hunks)
- src/meta-srv/src/handler/extract_stat_handler.rs (1 hunks)
- src/meta-srv/src/metasrv/builder.rs (3 hunks)
Additional comments not posted (7)
src/meta-srv/src/handler/extract_stat_handler.rs (3)
23-23
: Struct Definition ApprovedThe
ExtractStatHandler
struct is correctly defined as a unit struct, suitable for its purpose.
26-29
: Method Implementation Review:is_acceptable
The
is_acceptable
method correctly checks if the role isDatanode
, which aligns with the design intent of handling data node-specific statistics.
31-52
: Method Implementation Review:handle
The
handle
method efficiently checks for mailbox messages and skips processing if present, which is a good optimization given the context. The conversion of the request to aStat
object and the subsequent error handling are well-implemented.src/meta-srv/src/handler.rs (1)
46-46
: Module Addition ApprovedThe addition of the
extract_stat_handler
module is consistent with the introduction ofExtractStatHandler
in other parts of the codebase.src/meta-srv/src/metasrv/builder.rs (3)
365-365
: Handler Addition ApprovedThe addition of
ExtractStatHandler
to the builder is correctly implemented, ensuring the handler is integrated into the system.
368-368
: Handler Renaming ApprovedThe renaming of
CollectFrontendClusterInfoHandler
toCollectFlownodeClusterInfoHandler
is consistently applied and aligns with the PR's objectives to clarify handler names.
378-378
: Handler Addition ApprovedThe addition of
CollectStatsHandler
to the builder is correctly implemented, ensuring the handler is integrated into the system.
@coderabbitai Your summary is not accurate. For example, ExtractStatHandler was not newly introduced; I just renamed it. |
@fengjiachun, thank you for pointing that out. Upon reviewing the git history and diffs, I can confirm that Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check the git history for the file where `ExtractStatHandler` is mentioned to verify if it was newly created or renamed.
git log --follow -- src/meta-srv/src/handler/extract_stat_handler.rs
Length of output: 655 Script: #!/bin/bash
# Check the git diff for the commit where `src/meta-srv/src/handler/extract_stat_handler.rs` was introduced to see if it was renamed or moved from another file.
git diff ddc7a80f56885f70e935dd9831aabb9c68d17340 d38d5cd60735b95a432722d5e188f3d3fa13949c
Length of output: 22022 Learnings added
Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/meta-srv/src/handler.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/meta-srv/src/handler.rs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4236 +/- ##
==========================================
- Coverage 84.86% 84.59% -0.28%
==========================================
Files 1045 1046 +1
Lines 184365 184620 +255
==========================================
- Hits 156470 156179 -291
- Misses 27895 28441 +546 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/meta-srv/src/handler/collect_stats_handler.rs (2 hunks)
Additional comments not posted (6)
src/meta-srv/src/handler/collect_stats_handler.rs (6)
31-66
: Consider adding documentation forEpochStats
.Adding documentation comments for the
EpochStats
struct and its methods will improve code readability and maintainability./// Struct to manage statistics and epochs. struct EpochStats { stats: Vec<Stat>, epoch: Option<u64>, } impl EpochStats { /// Drains all statistics. #[inline] fn drain_all(&mut self) -> Vec<Stat> { self.stats.drain(..).collect() } /// Clears all statistics. #[inline] fn clear_stats(&mut self) { self.stats.clear(); } /// Pushes a new statistic. #[inline] fn push_stat(&mut self, stat: Stat) { self.stats.push(stat); } /// Returns the number of statistics. #[inline] fn len(&self) -> usize { self.stats.len() } /// Returns the current epoch. #[inline] fn epoch(&self) -> Option<u64> { self.epoch } /// Sets a new epoch. #[inline] fn set_epoch(&mut self, epoch: u64) { self.epoch = Some(epoch); } }
69-72
: Consider adding documentation forCollectStatsHandler
.Adding documentation comments for the
CollectStatsHandler
struct will improve code readability and maintainability./// Handler to collect and cache statistics. pub struct CollectStatsHandler { stats_cache: DashMap<DatanodeStatKey, EpochStats>, }
81-88
: Consider logging whencurrent_stat
isNone
.Adding a log statement when
current_stat
isNone
can help in debugging and monitoring.let Some(current_stat) = acc.stat.take() else { warn!("No current_stat received in heartbeat"); return Ok(HandleControl::Continue); };
96-112
: Consider simplifying the epoch comparison logic.The epoch comparison logic can be simplified for better readability.
let refresh = match epoch_stats.epoch() { Some(epoch) if current_stat.node_epoch > epoch => { // This node may have been redeployed. epoch_stats.clear_stats(); epoch_stats.set_epoch(current_stat.node_epoch); epoch_stats.push_stat(current_stat); true } Some(epoch) if current_stat.node_epoch == epoch => { epoch_stats.push_stat(current_stat); false } Some(_) => { warn!("Ignore stale heartbeat: {:?}", current_stat); false } None => { epoch_stats.set_epoch(current_stat.node_epoch); epoch_stats.push_stat(current_stat); // If the epoch is empty, it indicates that the current node sending the heartbeat // for the first time to the current meta leader, so it is necessary to save // the data to the KV store as soon as possible. true } };
18-21
: Ensure new imports are necessary and used.Verify that
PutRequest
andDashMap
are necessary and used within the code. Unused imports can be removed to keep the code clean.Verification successful
Imports are necessary and correctly used.
The imports
PutRequest
andDashMap
are used within the code and are necessary. No changes are needed.
PutRequest
is instantiated in the code.DashMap
is used as a type forstats_cache
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new imports in the file. # Test: Search for the usage of `PutRequest` and `DashMap`. Expect: Usage found. rg --type rust 'PutRequest|DashMap' src/meta-srv/src/handler/collect_stats_handler.rsLength of output: 236
147-215
: Ensure test coverage for edge cases.The test
test_handle_datanode_stats
should cover edge cases, such as handling stale heartbeats and large numbers of stats.
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?
CollectFlownodeClusterInfoHandler
into the pipeline, which will result in the inability to collect the cluster info of flownodes (this part of information for flownodes has not been enabled yet).CollectStatsHandler
toExtractStatHandler
PersistStatsHandler
toCollectStatsHandler
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor