add child manager aggregation logic by cjqzhao · Pull Request #18 · dfawley/tonic

cjqzhao · 2025-07-01T05:40:22Z

Add child manager aggregation logic

dfawley · 2025-07-01T20:34:32Z

grpc/src/client/load_balancing/child_manager.rs

+    // work_requests: Arc<Mutex<HashSet<Arc<T>>>>,
+    // work_scheduler: Arc<dyn WorkScheduler>,


Please delete all commented-out code.

dfawley · 2025-07-01T20:36:06Z

grpc/src/client/load_balancing/child_manager.rs

    fn resolve_child_controller(
        &mut self,
-        channel_controller: WrappedController,
+        channel_controller: &mut WrappedController,        


Please configure your editor to remove trailing whitespaces on save. And/or make sure you are running rustfmt (which I would expect would do that, too).

dfawley · 2025-07-01T20:37:02Z

grpc/src/client/load_balancing/child_manager.rs

+
+

Formatting-wise, please make sure to follow some standard practices, e.g. just one blank line between functions, no double-blank lines in the middle of a function, no blank lines between closing braces, etc.

dfawley · 2025-07-01T20:37:33Z

grpc/src/client/load_balancing/child_manager.rs

 }

-pub trait ChildIdentifier: PartialEq + Hash + Eq + Send + Sync + 'static {}
+use std::{sync::{atomic::{AtomicUsize, Ordering}}};


This should go above where the other use statements are.

dfawley · 2025-07-01T20:38:08Z

grpc/src/client/load_balancing/child_manager.rs

-pub trait ChildIdentifier: PartialEq + Hash + Eq + Send + Sync + 'static {}
+use std::{sync::{atomic::{AtomicUsize, Ordering}}};
+
+pub trait ChildIdentifier: PartialEq + Hash + Eq + Send + Sync + Display + 'static {}


I don't know that we want to enforce child identifiers to implement Display... Debug should be sufficient.

dfawley · 2025-07-01T21:07:09Z

grpc/src/client/load_balancing/child_manager.rs

+        // }
+    }
+
+    fn compare_prev_to_new_pickers(& self, old_pickers: &[Arc<dyn Picker>], new_pickers: &[Arc<dyn Picker>]) -> bool {


I don't think we need to do this... We can just always produce an update if any child updated. That should be good enough.

Will look into it! I remember that I was running into an issue with updated not being reliable which is why I added this but I definitely could have missed something

dfawley · 2025-07-01T21:09:51Z

grpc/src/client/load_balancing/child_manager.rs

    }
+
+    fn exit_idle(&mut self, channel_controller: &mut dyn ChannelController) {
+        todo!()


Call all children and exit_idle them?

Oh huh I wrote this but somehow didn't make it into the commit...

dfawley · 2025-07-01T21:10:53Z

grpc/src/client/load_balancing/child_manager.rs

    }
 }
+
+struct ReadyPickers {


Can we have just one RoundRobinPicker that holds these same fields?

dfawley · 2025-07-01T21:11:38Z

grpc/src/client/load_balancing/child_manager.rs

+    pub fn new() -> Self {
+        Self {
+            pickers: vec![],
+            next: AtomicUsize::new(0),
+        }
+    }
+
+    fn add_picker(&mut self, picker: Arc<dyn Picker>)  {
+        self.pickers.push(picker);
+    }
+
+    pub fn has_any(&self) -> bool {
+        !self.pickers.is_empty()
+    }


If possible, it would simplify if you only have a new(v: Vec<Arc<dyn Picker>>).

dfawley · 2025-07-01T21:12:39Z

grpc/src/client/load_balancing/child_manager.rs

+
+impl Picker for SchedulingIdlePicker {
+    fn pick(&self, _request: &Request) -> PickResult {
+        self.work_scheduler.schedule_work();


I don't believe this special case is needed. When you call into the idle picker of the child, it should be responsible for requesting work if it needs it.

Yes will delete.

dfawley · 2025-07-02T20:12:05Z

grpc/src/client/load_balancing/child_manager.rs

+    sent_connecting_state: bool,
+    aggregated_state: ConnectivityState,
+    last_ready_pickers: Vec<Arc<dyn Picker>>,
+


Please remove extraneous blank lines. Blank lines should be intentionally visually separating things.

dfawley · 2025-07-02T20:14:08Z

grpc/src/client/load_balancing/child_manager.rs

+    pub fn new(
+        update_sharder: Box<dyn ResolverUpdateSharder<T>>
+    ) -> Self {


This shouldn't need to get reformatted. How are you running rustfmt?

dfawley · 2025-07-02T20:14:34Z

grpc/src/client/load_balancing/child_manager.rs

+    pub fn new(
+        update_sharder: Box<dyn ResolverUpdateSharder<T>>
+    ) -> Self {
+        ChildManager {


Suggested change

ChildManager {

Self {

I think is preferred, as it was before.

dfawley · 2025-07-02T20:16:35Z

grpc/src/client/load_balancing/child_manager.rs

+        // let mut transient_failure_picker = TransientFailurePickers::new("error string I guess".to_string());
+        // let mut connecting_pickers = ConnectingPickers::new();


Commented-out code should always be removed.

dfawley · 2025-07-18T21:41:32Z

grpc/src/client/load_balancing/child_manager.rs

+    }
+
+    /// Called to aggregate states from children policies then returns a update.
+    pub fn aggregate_states(&mut self) -> Option<LbState> {


Why is this an Option? I would think it should always return a valid LbState, even if it's "TRANSIENT_FAILURE, "

dfawley · 2025-07-18T21:46:32Z

grpc/src/client/load_balancing/child_manager.rs

+                let should_update =
+                    !self.compare_prev_to_new_pickers(&self.last_ready_pickers, &pickers_vec);


Why does this exist? I thought this function would just produce the aggregate state / aggregate picker, always. Why wouldn't it want to do that?

dfawley · 2025-07-18T21:48:53Z

grpc/src/client/load_balancing/child_manager.rs

+            prev_state: ConnectivityState::Idle,
+            last_ready_pickers: Vec::new(),


This kind of state, I think, should generally be kept in a policy that uses the child manager nad not in the child manager itself.

dfawley · 2025-07-18T21:50:20Z

grpc/src/client/load_balancing/child_manager.rs

    }
+
+    fn exit_idle(&mut self, channel_controller: &mut dyn ChannelController) {
+        let child_idxes = mem::take(&mut *self.pending_work.lock().unwrap());


This isn't right. We want to call exit_idle on all children (or all idle children?) if this happens. Not just the ones that have requested work (which should be none of the idle ones).

dfawley · 2025-07-18T21:50:40Z

grpc/src/client/load_balancing/child_manager.rs


    fn update_picker(&mut self, update: LbState) {
-        self.picker_update = Some(update);
+        self.picker_update = Some(update.clone());


Why this change?

dfawley · 2025-07-18T21:52:04Z

grpc/src/client/load_balancing/child_manager.rs

+    fn add_picker(&mut self, picker: Arc<dyn Picker>) {
+        self.pickers.push(picker);
+    }


Can we make it so all the pickers are known when new is called instead?

Ideally we set next initially to a random value in the range of the number of entries.

dfawley · 2025-07-18T21:53:03Z

grpc/src/client/load_balancing/child_manager.rs

+        if len == 0 {
+            return PickResult::Queue;
+        }


Can this special case be handled by having a queuing picker instead, and not using this with empty lists?

add child manager aggregation logic

1bdeab1

dfawley reviewed Jul 1, 2025

View reviewed changes

dfawley reviewed Jul 2, 2025

View reviewed changes

cjqzhao force-pushed the childmanagerstates branch from a76ed88 to 1bdeab1 Compare July 2, 2025 22:34

cjqzhao added 2 commits July 7, 2025 18:18

fixed some logic for review

f7537e6

fixed transient failure error message

69971f7

dfawley reviewed Jul 18, 2025

View reviewed changes

implemented some changes

aa1f715

		// work_requests: Arc<Mutex<HashSet<Arc<T>>>>,
		// work_scheduler: Arc<dyn WorkScheduler>,

		// let mut transient_failure_picker = TransientFailurePickers::new("error string I guess".to_string());
		// let mut connecting_pickers = ConnectingPickers::new();

		let should_update =
		!self.compare_prev_to_new_pickers(&self.last_ready_pickers, &pickers_vec);

		prev_state: ConnectivityState::Idle,
		last_ready_pickers: Vec::new(),

Comments

Conversation

cjqzhao commented Jul 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants