From 1eb96dc7fd70d72849b638aafd39de815521b540 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 22 Mar 2017 12:19:03 -0700 Subject: [PATCH 1/2] Fix semantics of LB policy selection. --- doc/service_config.md | 20 ++++++++++++++------ src/core/ext/client_channel/client_channel.c | 18 ++++++++---------- src/core/ext/lb_policy/grpclb/grpclb.c | 8 ++++---- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/doc/service_config.md b/doc/service_config.md index ecc23817d12..c5d4ef42b19 100644 --- a/doc/service_config.md +++ b/doc/service_config.md @@ -13,12 +13,20 @@ The service config is a JSON string of the following form: ``` { // Load balancing policy name. - // Supported values are 'round_robin' and 'grpclb'. - // Optional; if unset, the default behavior is pick the first available - // backend. - // Note that if the resolver returns only balancer addresses and no - // backend addresses, gRPC will always use the 'grpclb' policy, - // regardless of what this field is set to. + // Currently, the only selectable client-side policy provided with gRPC + // is 'round_robin', but third parties may add their own policies. + // This field is optional; if unset, the default behavior is to pick + // the first available backend. + // If the policy name is set via the client API, that value overrides + // the value specified here. + // + // Note that if the resolver returns at least one balancer address (as + // opposed to backend addresses), gRPC will use grpclb (see + // https://github.com/grpc/grpc/blob/master/doc/load-balancing.md), + // regardless of what LB policy is requested either here or via the + // client API. However, if the resolver returns backend addresses as + // well as balancer addresses, the client may fall back to the requested + // policy if it is unable to reach any of the grpclb load balancers. 'loadBalancingPolicy': string, // Per-method configuration. Optional. diff --git a/src/core/ext/client_channel/client_channel.c b/src/core/ext/client_channel/client_channel.c index 00c20913b0f..7df211d0000 100644 --- a/src/core/ext/client_channel/client_channel.c +++ b/src/core/ext/client_channel/client_channel.c @@ -368,27 +368,25 @@ static void on_resolver_result_changed_locked(grpc_exec_ctx *exec_ctx, GPR_ASSERT(channel_arg->type == GRPC_ARG_STRING); lb_policy_name = channel_arg->value.string; } - // Special case: If all of the addresses are balancer addresses, - // assume that we should use the grpclb policy, regardless of what the - // resolver actually specified. + // Special case: If at least one balancer address is present, we use + // the grpclb policy, regardless of what the resolver actually specified. channel_arg = grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); if (channel_arg != NULL) { GPR_ASSERT(channel_arg->type == GRPC_ARG_POINTER); grpc_lb_addresses *addresses = channel_arg->value.pointer.p; - bool found_backend_address = false; + bool found_balancer_address = false; for (size_t i = 0; i < addresses->num_addresses; ++i) { - if (!addresses->addresses[i].is_balancer) { - found_backend_address = true; + if (addresses->addresses[i].is_balancer) { + found_balancer_address = true; break; } } - if (!found_backend_address) { + if (found_balancer_address) { if (lb_policy_name != NULL && strcmp(lb_policy_name, "grpclb") != 0) { gpr_log(GPR_INFO, - "resolver requested LB policy %s but provided only balancer " - "addresses, no backend addresses -- forcing use of grpclb LB " - "policy", + "resolver requested LB policy %s but provided at least one " + "balancer address -- forcing use of grpclb LB policy", lb_policy_name); } lb_policy_name = "grpclb"; diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index d612591f2eb..e0e5e28eb6f 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -841,10 +841,10 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, /* Count the number of gRPC-LB addresses. There must be at least one. * TODO(roth): For now, we ignore non-balancer addresses, but in the * future, we may change the behavior such that we fall back to using - * the non-balancer addresses if we cannot reach any balancers. At that - * time, this should be changed to allow a list with no balancer addresses, - * since the resolver might fail to return a balancer address even when - * this is the right LB policy to use. */ + * the non-balancer addresses if we cannot reach any balancers. In the + * fallback case, we should use the LB policy indicated by + * GRPC_ARG_LB_POLICY_NAME (although if that specifies grpclb or is + * unset, we should default to pick_first). */ const grpc_arg *arg = grpc_channel_args_find(args->args, GRPC_ARG_LB_ADDRESSES); GPR_ASSERT(arg != NULL && arg->type == GRPC_ARG_POINTER); From e3ec4b2c287b480d61ddaa2b461cf6d468519313 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Fri, 24 Mar 2017 07:33:38 -0700 Subject: [PATCH 2/2] Clarify wording. --- doc/service_config.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/service_config.md b/doc/service_config.md index c5d4ef42b19..8039fcad095 100644 --- a/doc/service_config.md +++ b/doc/service_config.md @@ -24,9 +24,10 @@ The service config is a JSON string of the following form: // opposed to backend addresses), gRPC will use grpclb (see // https://github.com/grpc/grpc/blob/master/doc/load-balancing.md), // regardless of what LB policy is requested either here or via the - // client API. However, if the resolver returns backend addresses as - // well as balancer addresses, the client may fall back to the requested - // policy if it is unable to reach any of the grpclb load balancers. + // client API. However, if the resolver returns at least one backend + // address in addition to the balancer address(es), the client may fall + // back to the requested policy if it is unable to reach any of the + // grpclb load balancers. 'loadBalancingPolicy': string, // Per-method configuration. Optional.