flow-filter clean-up: Remove is_initiator from FlowSummary - #1784
Conversation
No need to keep a separate field in FlowSummary for the value of the initiator flag, we can trivially retrieve it from the flow info (attached to the FlowSummary) without locking. Fixes: 5d68fd1 ("feat(flow-filter): Use new flow flag to avoid duplicate context lookup") Suggested-by: Fredi Raspall <fredi@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughFlow revalidation now derives initiator status from ChangesFlow revalidation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This PR removes a redundant FlowSummary field and updates its related test usage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR removes the redundant is_initiator field from the internal FlowSummary helper type in flow-filter, instead deriving the initiator direction directly from the attached FlowInfo flags (avoiding any additional locking for that value).
Changes:
- Removed
is_initiator: boolfromFlowSummaryand stopped populating it when building summaries. - Replaced direct field access with a small
FlowSummary::is_initiator()accessor that reads fromFlowInfoflags. - Updated property-based tests that construct
FlowSummaryliterals to match the new struct layout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| flow-filter/src/lib.rs | Drops the cached initiator field from FlowSummary and derives initiator-ness from FlowInfo flags via a helper method. |
| flow-filter/src/tests.rs | Updates test construction of FlowSummary to remove the deleted is_initiator field. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
No need to keep a separate field in FlowSummary for the value of the initiator flag, we can trivially retrieve it from the flow info (attached to the FlowSummary) without locking.
Reported by Fredi during the review for #1713, but the PR got merged before I got a chance to address it.