From a60226726ae6076916db0e1d616338a1a211770d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 18 Oct 2018 10:49:25 -0700 Subject: reviewer feedback --- .../ext/filters/client_channel/client_channel.cc | 122 ++++++++++++--------- 1 file changed, 69 insertions(+), 53 deletions(-) (limited to 'src/core/ext/filters/client_channel/client_channel.cc') diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 95d1f41094..64e206ec63 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -131,8 +131,8 @@ typedef struct client_channel_channel_data { grpc_core::UniquePtr info_service_config_json; /* backpointer to grpc_channel's channelz node */ grpc_core::channelz::ClientChannelNode* channelz_channel; - /* caches if the last resolution event led to zero addresses */ - bool previous_resolution_zero_num_addresses; + /* caches if the last resolution event contained addresses */ + bool previous_resolution_contained_addresses; } channel_data; typedef struct { @@ -403,6 +403,8 @@ static void request_reresolution_locked(void* arg, grpc_error* error) { chand->lb_policy->SetReresolutionClosureLocked(&args->closure); } +using TraceStringVector = grpc_core::InlinedVector; + // Creates a new LB policy, replacing any previous one. // If the new policy is created successfully, sets *connectivity_state and // *connectivity_error to its initial connectivity state; otherwise, @@ -410,7 +412,7 @@ static void request_reresolution_locked(void* arg, grpc_error* error) { static void create_new_lb_policy_locked( channel_data* chand, char* lb_policy_name, grpc_connectivity_state* connectivity_state, - grpc_error** connectivity_error) { + grpc_error** connectivity_error, TraceStringVector* trace_strings) { grpc_core::LoadBalancingPolicy::Args lb_policy_args; lb_policy_args.combiner = chand->combiner; lb_policy_args.client_channel_factory = chand->client_channel_factory; @@ -420,11 +422,21 @@ static void create_new_lb_policy_locked( lb_policy_name, lb_policy_args); if (GPR_UNLIKELY(new_lb_policy == nullptr)) { gpr_log(GPR_ERROR, "could not create LB policy \"%s\"", lb_policy_name); + if (chand->channelz_channel != nullptr) { + char* str; + gpr_asprintf(&str, "Could not create LB policy \'%s\'", lb_policy_name); + trace_strings->push_back(str); + } } else { if (grpc_client_channel_trace.enabled()) { gpr_log(GPR_INFO, "chand=%p: created new LB policy \"%s\" (%p)", chand, lb_policy_name, new_lb_policy.get()); } + if (chand->channelz_channel != nullptr) { + char* str; + gpr_asprintf(&str, "Created new LB policy \'%s\'", lb_policy_name); + trace_strings->push_back(str); + } // Swap out the LB policy and update the fds in // chand->interested_parties. if (chand->lb_policy != nullptr) { @@ -499,6 +511,51 @@ get_service_config_from_resolver_result_locked(channel_data* chand) { return grpc_core::UniquePtr(gpr_strdup(service_config_json)); } +static void check_for_important_resolution_change( + channel_data* chand, TraceStringVector* trace_strings) { + int resolution_contains_addresses = false; + const grpc_arg* channel_arg = + grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); + if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) { + grpc_lb_addresses* addresses = + static_cast(channel_arg->value.pointer.p); + if (addresses->num_addresses > 0) { + resolution_contains_addresses = true; + } + } + if (!resolution_contains_addresses && + chand->previous_resolution_contained_addresses) { + trace_strings->push_back(gpr_strdup("Address list became empty")); + } else if (resolution_contains_addresses && + !chand->previous_resolution_contained_addresses) { + trace_strings->push_back(gpr_strdup("Address list became non-empty")); + } + chand->previous_resolution_contained_addresses = + resolution_contains_addresses; +} + +static void concatenate_and_add_channel_trace( + channel_data* chand, TraceStringVector* trace_strings) { + if (!trace_strings->empty()) { + gpr_strvec v; + gpr_strvec_init(&v); + gpr_strvec_add(&v, gpr_strdup("Resolution event: ")); + bool is_first = 1; + for (size_t i = 0; i < trace_strings->size(); ++i) { + if (!is_first) gpr_strvec_add(&v, gpr_strdup(", ")); + is_first = false; + gpr_strvec_add(&v, (*trace_strings)[i]); + } + char* flat; + size_t flat_len = 0; + flat = gpr_strvec_flatten(&v, &flat_len); + chand->channelz_channel->AddTraceEvent( + grpc_core::channelz::ChannelTrace::Severity::Info, + grpc_slice_new(flat, flat_len, gpr_free)); + gpr_strvec_destroy(&v); + } +} + // Callback invoked when a resolver result is available. static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { channel_data* chand = static_cast(arg); @@ -529,7 +586,7 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { // (d) Address resolution that causes a new LB policy to be created. // // we track a list of strings to eventually be concatenated and traced. - grpc_core::InlinedVector trace_strings; + TraceStringVector trace_strings; grpc_connectivity_state connectivity_state = GRPC_CHANNEL_TRANSIENT_FAILURE; grpc_error* connectivity_error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("No load balancing policy"); @@ -564,12 +621,8 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { } else { // Instantiate new LB policy. create_new_lb_policy_locked(chand, lb_policy_name.get(), - &connectivity_state, &connectivity_error); - if (chand->channelz_channel != nullptr) { - char* str; - gpr_asprintf(&str, "Switched LB policy to %s", lb_policy_name.get()); - trace_strings.push_back(str); - } + &connectivity_state, &connectivity_error, + &trace_strings); } // Find service config. grpc_core::UniquePtr service_config_json = @@ -579,54 +632,17 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { // only thing that modifies its value, and it can only be invoked // once at any given time. if (chand->channelz_channel != nullptr) { - if ((service_config_json == nullptr && - chand->info_service_config_json != nullptr) || - (service_config_json != nullptr && - chand->info_service_config_json == nullptr) || + if (((service_config_json == nullptr) != + (chand->info_service_config_json == nullptr)) || (service_config_json != nullptr && - chand->info_service_config_json != nullptr && strcmp(service_config_json.get(), chand->info_service_config_json.get()) != 0)) { // TODO(ncteisen): might be worth somehow including a snippet of the // config in the trace, at the risk of bloating the trace logs. - trace_strings.push_back(gpr_strdup("Service config reloaded")); - } - int zero_num_addresses = true; - const grpc_arg* channel_arg = - grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); - if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) { - grpc_lb_addresses* addresses = - static_cast(channel_arg->value.pointer.p); - if (addresses->num_addresses > 0) { - zero_num_addresses = false; - } - } - if (zero_num_addresses && - !chand->previous_resolution_zero_num_addresses) { - trace_strings.push_back(gpr_strdup("Address list became empty")); - } else if (!zero_num_addresses && - chand->previous_resolution_zero_num_addresses) { - trace_strings.push_back(gpr_strdup("Address list became non-empty")); - } - chand->previous_resolution_zero_num_addresses = zero_num_addresses; - if (!trace_strings.empty()) { - gpr_strvec v; - gpr_strvec_init(&v); - gpr_strvec_add(&v, gpr_strdup("Resolution event: ")); - bool is_first = 1; - for (size_t i = 0; i < trace_strings.size(); ++i) { - if (!is_first) gpr_strvec_add(&v, gpr_strdup(", ")); - is_first = false; - gpr_strvec_add(&v, trace_strings[i]); - } - char* flat; - size_t flat_len = 0; - flat = gpr_strvec_flatten(&v, &flat_len); - chand->channelz_channel->AddTraceEvent( - grpc_core::channelz::ChannelTrace::Severity::Info, - grpc_slice_new(flat, flat_len, gpr_free)); - gpr_strvec_destroy(&v); + trace_strings.push_back(gpr_strdup("Service config changed")); } + check_for_important_resolution_change(chand, &trace_strings); + concatenate_and_add_channel_trace(chand, &trace_strings); } // Swap out the data used by cc_get_channel_info(). gpr_mu_lock(&chand->info_mu); @@ -796,7 +812,7 @@ static grpc_error* cc_init_channel_elem(grpc_channel_element* elem, arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_ENABLE_RETRIES); chand->enable_retries = grpc_channel_arg_get_bool(arg, true); chand->channelz_channel = nullptr; - chand->previous_resolution_zero_num_addresses = true; + chand->previous_resolution_contained_addresses = false; // Record client channel factory. arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_CLIENT_CHANNEL_FACTORY); -- cgit v1.2.3