From 5132d0e6092d34840e583dac0ef0dd01f4226a63 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 13 Oct 2017 13:20:52 -0700 Subject: [PATCH] Code review changes. --- .../lb_policy/pick_first/pick_first.cc | 8 ++++---- .../lb_policy/round_robin/round_robin.cc | 7 +++---- .../client_channel/lb_policy/subchannel_list.cc | 12 +++++------- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index d861746999d..025ee290813 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc @@ -44,9 +44,9 @@ typedef struct { grpc_lb_policy base; /** all our subchannels */ grpc_lb_subchannel_list *subchannel_list; - /** Latest pending subchannel list. */ + /** latest pending subchannel list */ grpc_lb_subchannel_list *latest_pending_subchannel_list; - /** Selected subchannel in \a subchannel_list. */ + /** selected subchannel in \a subchannel_list */ grpc_lb_subchannel_data *selected; /** have we started picking? */ bool started_picking; @@ -351,7 +351,7 @@ static void pf_update_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, } // If we've started picking, start trying to connect to the first // subchannel in the new list. - if (p->started_picking && subchannel_list->num_subchannels > 0) { + if (p->started_picking) { grpc_lb_subchannel_list_ref_for_connectivity_watch( subchannel_list, "connectivity_watch+update"); grpc_lb_subchannel_data_start_connectivity_watch( @@ -396,7 +396,7 @@ static void pf_connectivity_changed_locked(grpc_exec_ctx *exec_ctx, void *arg, // either the current or latest pending subchannel lists. GPR_ASSERT(sd->subchannel_list == p->subchannel_list || sd->subchannel_list == p->latest_pending_subchannel_list); - // Update state counters. + // Update state. sd->curr_connectivity_state = sd->pending_connectivity_state_unsafe; // Handle updates for the currently selected subchannel. if (p->selected == sd) { diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc index 499631f1b18..67361bfe5d9 100644 --- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc +++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc @@ -436,7 +436,7 @@ static void rr_connectivity_changed_locked(grpc_exec_ctx *exec_ctx, void *arg, grpc_lb_subchannel_data_stop_connectivity_watch(exec_ctx, sd); grpc_lb_subchannel_data_unref_subchannel(exec_ctx, sd, "rr_sl_shutdown"); grpc_lb_subchannel_list_unref_for_connectivity_watch( - exec_ctx, sd->subchannel_list, "rr_shutdown"); + exec_ctx, sd->subchannel_list, "rr_sl_shutdown"); return; } // If we're still here, the notification must be for a subchannel in @@ -601,7 +601,7 @@ static void rr_update_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, return; } if (p->started_picking) { - if (p->latest_pending_subchannel_list != NULL && p->started_picking) { + if (p->latest_pending_subchannel_list != NULL) { if (GRPC_TRACER_ON(grpc_lb_round_robin_trace)) { gpr_log(GPR_DEBUG, "[RR %p] Shutting down latest pending subchannel list %p, " @@ -610,8 +610,7 @@ static void rr_update_locked(grpc_exec_ctx *exec_ctx, grpc_lb_policy *policy, (void *)subchannel_list); } grpc_lb_subchannel_list_shutdown_and_unref( - exec_ctx, p->latest_pending_subchannel_list, - "sl_outdated_dont_smash"); + exec_ctx, p->latest_pending_subchannel_list, "sl_outdated"); } p->latest_pending_subchannel_list = subchannel_list; for (size_t i = 0; i < subchannel_list->num_subchannels; ++i) { diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc index db057e295d0..9a7ccedac17 100644 --- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc +++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc @@ -103,11 +103,11 @@ grpc_lb_subchannel_list *grpc_lb_subchannel_list_create( gpr_ref_init(&subchannel_list->refcount, 1); subchannel_list->subchannels = (grpc_lb_subchannel_data *)gpr_zalloc( sizeof(grpc_lb_subchannel_data) * addresses->num_addresses); - /* We need to remove the LB addresses in order to be able to compare the - * subchannel keys of subchannels from a different batch of addresses. */ + // We need to remove the LB addresses in order to be able to compare the + // subchannel keys of subchannels from a different batch of addresses. static const char *keys_to_remove[] = {GRPC_ARG_SUBCHANNEL_ADDRESS, GRPC_ARG_LB_ADDRESSES}; - /* Create subchannels for addresses in the update. */ + // Create a subchannels for each address. grpc_subchannel_args sc_args; size_t subchannel_index = 0; for (size_t i = 0; i < addresses->num_addresses; i++) { @@ -174,10 +174,8 @@ grpc_lb_subchannel_list *grpc_lb_subchannel_list_create( GRPC_CLOSURE_INIT(&sd->connectivity_changed_closure, connectivity_changed_cb, sd, grpc_combiner_scheduler(args->combiner)); - /* use some sentinel value outside of the range of - * grpc_connectivity_state to signal an undefined previous state. We - * won't be referring to this value again and it'll be overwritten after - * the first call to rr_connectivity_changed_locked */ + // Use some sentinel value outside of the range of + // grpc_connectivity_state to signal an undefined previous state. sd->prev_connectivity_state = GRPC_CHANNEL_INIT; sd->curr_connectivity_state = subchannel_connectivity_state; sd->user_data_vtable = addresses->user_data_vtable;