feat: store peer info in TableFlowValue
#4280
Conversation
WalkthroughThe recent changes involve modifying the handling of 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: 4
Outside diff range and nitpick comments (8)
src/common/meta/src/key/flow/flow_route.rs (1)
190-190: Add missing parameter documentation.The
partition_idparameter is added to thebuild_create_txnfunction but is not documented. Ensure all function parameters are documented for clarity.src/common/meta/src/key/flow/table_flow.rs (3)
20-20: Organize imports.Group related imports together for better readability. Consider grouping
serdeimports together andcrateimports together.Also applies to: 26-26, 29-29
171-174: Add documentation forTableFlowValue.The
TableFlowValuestruct is missing Rustdoc comments. Add documentation to explain the purpose and usage of this struct.
199-202: Add missing documentation forflowsfunction parameters.The
flowsfunction is missing documentation for its parameters. Ensure all parameters are documented for clarity.src/common/meta/src/instruction.rs (2)
28-28: Organize imports.Group related imports together for better readability. Consider grouping
serdeimports together andcrateimports together.
173-173: Add documentation forflownode_peersfield.The
flownode_peersfield in theCreateFlowstruct is missing documentation. Add documentation to explain the purpose and usage of this field.src/common/meta/src/cache/flow/table_flownode.rs (2)
15-15: Organize imports.Group related imports together for better readability. Consider grouping
stdimports together andcrateimports together.Also applies to: 29-29
32-32: Update type alias documentation.The
FlownodeSettype alias should have updated documentation to reflect the change fromHashSettoArc<HashMap>.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/common/meta/src/cache/flow/table_flownode.rs (11 hunks)
- src/common/meta/src/ddl/create_flow.rs (1 hunks)
- src/common/meta/src/instruction.rs (2 hunks)
- src/common/meta/src/key.rs (2 hunks)
- src/common/meta/src/key/flow.rs (4 hunks)
- src/common/meta/src/key/flow/flow_route.rs (1 hunks)
- src/common/meta/src/key/flow/table_flow.rs (4 hunks)
- src/operator/src/insert.rs (1 hunks)
Additional comments not posted (6)
src/common/meta/src/cache/flow/table_flownode.rs (1)
108-115: Improve error handling inhandle_drop_flow.Add context to the error handling in the
handle_drop_flowfunction for better debugging.- async |entry: Option<moka::Entry<u32, Arc<HashMap<u64, _>>>>| match entry { - Some(entry) => { - let mut set = entry.into_value().as_ref().clone(); - for flownode_id in flownode_ids { - set.remove(flownode_id); - } - Op::Put(Arc::new(set)) - } - None => { - // Do nothing - Op::Nop - } - }, + async |entry: Option<moka::Entry<u32, Arc<HashMap<u64, _>>>>| { + if let Some(entry) = entry { + let mut set = entry.into_value().as_ref().clone(); + for flownode_id in flownode_ids { + set.remove(flownode_id); + } + Op::Put(Arc::new(set)) + } else { + // Do nothing + Op::Nop + } + },Likely invalid or redundant comment.
src/common/meta/src/ddl/create_flow.rs (1)
197-197: LGTM! But verify the usage offlownode_peers.The code changes are approved.
However, ensure that all instances of
flownode_idshave been correctly replaced withflownode_peers.src/common/meta/src/key/flow.rs (1)
171-176: LGTM! But verify the transformation logic.The code changes are approved.
However, ensure that the transformation logic for
flow_routestoTableFlowValueis consistent throughout the codebase.Verification successful
Let's create a new shell script to accurately search for the transformation logic related to
TableFlowValue { peer: route.peer }by escaping the curly braces.
To ensure consistency in the transformation logic throughout the codebase, let's search for all instances where
TableFlowValueis used.
To verify the consistency of the transformation logic, let's extract the specific lines where
TableFlowValueis instantiated inflow.rsandtable_flow.rs.
To ensure consistency in the transformation logic, let's extract and review the lines around the instances of
TableFlowValueinflow.rsandtable_flow.rs.
Transformation Logic Verified
The transformation logic for
flow_routestoTableFlowValue { peer: route.peer }is consistent throughout the codebase. The instances inflow.rsand the definition intable_flow.rsalign correctly.
flow.rs:
- Line 173: Transformation logic
- Lines 438-444: Consistent usage of
TableFlowValue { peer: Peer::empty(x) }
table_flow.rs:
- Line 172: Definition of
TableFlowValue- Line 179: Usage in
table_flow_decoder- Lines 202, 221: Usage in methods
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the transformation logic for `flow_routes` to `TableFlowValue`. # Test: Search for the transformation logic. Expect: Consistent usage of `TableFlowValue { peer: route.peer }`. rg --type rust $'TableFlowValue { peer: route.peer }'Length of output: 189
Script:
#!/bin/bash # Description: Verify the transformation logic for `flow_routes` to `TableFlowValue`. # Test: Search for the transformation logic. Expect: Consistent usage of `TableFlowValue { peer: route.peer }`. rg --type rust 'TableFlowValue \{ peer: route.peer \}'Length of output: 186
Script:
#!/bin/bash # Description: Verify the usage of `TableFlowValue` throughout the codebase. # Test: Search for all instances of `TableFlowValue`. Expect: Consistent transformation logic. rg --type rust 'TableFlowValue'Length of output: 1010
Script:
#!/bin/bash # Description: Extract lines where `TableFlowValue` is instantiated for verification. # Extract lines from `flow.rs` rg --type rust 'TableFlowValue' src/common/meta/src/key/flow.rs -n # Extract lines from `table_flow.rs` rg --type rust 'TableFlowValue' src/common/meta/src/key/flow/table_flow.rs -nLength of output: 704
Script:
#!/bin/bash # Description: Extract surrounding lines where `TableFlowValue` is instantiated for detailed verification. # Extract surrounding lines from `flow.rs` rg --type rust 'TableFlowValue' src/common/meta/src/key/flow.rs -A 3 -B 3 # Extract surrounding lines from `table_flow.rs` rg --type rust 'TableFlowValue' src/common/meta/src/key/flow/table_flow.rs -A 3 -B 3Length of output: 2122
src/operator/src/insert.rs (1)
344-345: LGTM! But verify the usage offlownode_peers.The code changes are approved.
However, ensure that all instances of
flownode_idshave been correctly replaced withflownode_peers.src/common/meta/src/key.rs (2)
115-115: Addition ofTableFlowValuetoimpl_table_meta_value!macro is correct.The addition follows the existing pattern and ensures that
TableFlowValueimplements theTableMetaValuetrait.
1198-1198: Addition ofTableFlowValuetoimpl_table_meta_value!macro is correct.The addition follows the existing pattern and ensures that
TableFlowValueimplements theTableMetaValuetrait.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4280 +/- ##
==========================================
- Coverage 84.94% 84.69% -0.25%
==========================================
Files 1057 1058 +1
Lines 187517 188109 +592
==========================================
+ Hits 159279 159327 +48
- Misses 28238 28782 +544 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/common/meta/src/cache/flow/table_flownode.rs (11 hunks)
- src/common/meta/src/ddl/create_flow.rs (1 hunks)
- src/common/meta/src/instruction.rs (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/common/meta/src/cache/flow/table_flownode.rs
- src/common/meta/src/ddl/create_flow.rs
- src/common/meta/src/instruction.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?
Storing peer info in
FlowTableValuewhich reduce one query when mirror insertion.The original steps:
FlowIdsPeersChecklist
Summary by CodeRabbit
New Features
Bug Fixes
FlowRouteManagerto ensure accurate flow partition handling.Performance Improvements
HashSettoArc<HashMap>for flownode storage, which improves data handling efficiency.Developer Experience