Conversation
Reviewer's GuideThis PR extracts and encapsulates the fairness counting and yielding behavior into a new Fairness helper module, refactors ConnectionActor to delegate all priority-draining logic to this helper, and adds targeted unit tests for the new component. Class diagram for extracted Fairness helper and refactored ConnectionActorclassDiagram
class FairnessConfig {
+max_high_before_low: usize
+time_slice: Option<Duration>
}
class Fairness {
-config: FairnessConfig
-high_counter: usize
-high_start: Option<Instant>
+new(config: FairnessConfig)
+set_config(config: FairnessConfig)
+after_high()
+should_yield() bool
+after_low()
+reset()
}
class ConnectionActor {
-fairness: Fairness
+set_fairness(fairness: FairnessConfig)
+after_high()
+after_low()
}
FairnessConfig <.. Fairness : uses
Fairness <.. ConnectionActor : used by
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughRefactor the fairness handling in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectionActor
participant Fairness
User->>ConnectionActor: Process high-priority frame
ConnectionActor->>Fairness: after_high()
ConnectionActor->>Fairness: should_yield()
alt Should yield
Fairness-->>ConnectionActor: true
ConnectionActor->>User: Yield to low-priority
else Continue
Fairness-->>ConnectionActor: false
ConnectionActor->>User: Continue high-priority
end
User->>ConnectionActor: Process low-priority frame
ConnectionActor->>Fairness: after_low()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs⚙️ CodeRabbit Configuration File
Files:
🔇 Additional comments (15)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider injecting a clock or time source into Fairness instead of calling Instant::now() directly to make timing logic more testable and decoupled from tokio.
- You might rename Fairness methods (e.g. should_yield → should_yield_to_low_priority) or the struct itself to make the intent clearer when reading ConnectionActor code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider injecting a clock or time source into Fairness instead of calling Instant::now() directly to make timing logic more testable and decoupled from tokio.
- You might rename Fairness methods (e.g. should_yield → should_yield_to_low_priority) or the struct itself to make the intent clearer when reading ConnectionActor code.
## Individual Comments
### Comment 1
<location> `src/fairness.rs:39` </location>
<code_context>
+ }
+ }
+
+ pub(crate) fn should_yield(&self) -> bool {
+ let threshold_hit = self.config.max_high_before_low > 0
+ && self.high_counter >= self.config.max_high_before_low;
</code_context>
<issue_to_address>
Consider refactoring the should_yield method to use early-return if-let patterns and consolidating after_low and reset into a shared helper for clarity.
You can flatten the two checks and avoid the `zip…is_some_and` trick with a straightforward early-return `if let` pattern. This makes each “hit” condition explicit and keeps the method under 10 lines:
```rust
impl Fairness {
pub(crate) fn should_yield(&self) -> bool {
// 1) threshold hit?
if self.config.max_high_before_low > 0
&& self.high_counter >= self.config.max_high_before_low
{
return true;
}
// 2) time‐slice hit?
if let (Some(slice), Some(start)) = (&self.config.time_slice, &self.high_start) {
if start.elapsed() >= *slice {
return true;
}
}
false
}
}
```
Optionally, since `after_low` is just `reset()`, you can collapse them by inlining the shared logic into a private helper and calling it from both places:
```rust
impl Fairness {
fn clear(&mut self) {
self.high_counter = 0;
self.high_start = None;
}
pub(crate) fn after_low(&mut self) {
self.clear();
}
pub(crate) fn reset(&mut self) {
self.clear();
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| pub(crate) fn should_yield(&self) -> bool { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the should_yield method to use early-return if-let patterns and consolidating after_low and reset into a shared helper for clarity.
You can flatten the two checks and avoid the zip…is_some_and trick with a straightforward early-return if let pattern. This makes each “hit” condition explicit and keeps the method under 10 lines:
impl Fairness {
pub(crate) fn should_yield(&self) -> bool {
// 1) threshold hit?
if self.config.max_high_before_low > 0
&& self.high_counter >= self.config.max_high_before_low
{
return true;
}
// 2) time‐slice hit?
if let (Some(slice), Some(start)) = (&self.config.time_slice, &self.high_start) {
if start.elapsed() >= *slice {
return true;
}
}
false
}
}Optionally, since after_low is just reset(), you can collapse them by inlining the shared logic into a private helper and calling it from both places:
impl Fairness {
fn clear(&mut self) {
self.high_counter = 0;
self.high_start = None;
}
pub(crate) fn after_low(&mut self) {
self.clear();
}
pub(crate) fn reset(&mut self) {
self.clear();
}
}![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
FairnesshelperFairnessinConnectionActorTesting
make fmtmake lintmake testmake markdownlintmake nixie(fails: ENOENT reading puppeteer files)https://chatgpt.com/codex/tasks/task_e_688bc8dc13608322880c81f8c36d2dd3
Summary by Sourcery
Extract fairness tracking into a dedicated Fairness component, integrate it into ConnectionActor to simplify high/low queue balancing, and validate the behavior with unit tests.
New Features:
Enhancements:
Tests: