From 544564e7f66865fb9502a2d0d2e959414524601d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 28 Sep 2016 14:53:34 -0700 Subject: [PATCH 1/5] Make initial connect retry backoff configurable --- include/grpc/impl/codegen/grpc_types.h | 3 ++ src/core/ext/client_config/subchannel.c | 53 ++++++++++++------------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index e5a82883be5..f291751305f 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -153,6 +153,9 @@ typedef struct { #define GRPC_ARG_SECONDARY_USER_AGENT_STRING "grpc.secondary_user_agent" /** The maximum time between subsequent connection attempts, in ms */ #define GRPC_ARG_MAX_RECONNECT_BACKOFF_MS "grpc.max_reconnect_backoff_ms" +/** The time between the first and second connection attempts, in ms */ +#define GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS \ + "grpc.initial_reconnect_backoff_ms" /* The caller of the secure_channel_create functions may override the target name used for SSL host name checking using this channel argument which is of type \a GRPC_ARG_STRING. This *should* be used for testing only. diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index d089cd4399e..ebf13109586 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -331,41 +331,40 @@ grpc_subchannel *grpc_subchannel_create(grpc_exec_ctx *exec_ctx, grpc_closure_init(&c->connected, subchannel_connected, c); grpc_connectivity_state_init(&c->state_tracker, GRPC_CHANNEL_IDLE, "subchannel"); - gpr_backoff_init(&c->backoff_state, - GRPC_SUBCHANNEL_RECONNECT_BACKOFF_MULTIPLIER, - GRPC_SUBCHANNEL_RECONNECT_JITTER, - GRPC_SUBCHANNEL_INITIAL_CONNECT_BACKOFF_SECONDS * 1000, - GRPC_SUBCHANNEL_RECONNECT_MAX_BACKOFF_SECONDS * 1000); + int initial_backoff_ms = + GRPC_SUBCHANNEL_INITIAL_CONNECT_BACKOFF_SECONDS * 1000; + int max_backoff_ms = GRPC_SUBCHANNEL_RECONNECT_MAX_BACKOFF_SECONDS * 1000; + bool fixed_reconnect_backoff = false; if (c->args) { for (size_t i = 0; i < c->args->num_args; i++) { if (0 == strcmp(c->args->args[i].key, "grpc.testing.fixed_reconnect_backoff")) { GPR_ASSERT(c->args->args[i].type == GRPC_ARG_INTEGER); - gpr_backoff_init(&c->backoff_state, 1.0, 0.0, - c->args->args[i].value.integer, - c->args->args[i].value.integer); - } - if (0 == - strcmp(c->args->args[i].key, GRPC_ARG_MAX_RECONNECT_BACKOFF_MS)) { - if (c->args->args[i].type == GRPC_ARG_INTEGER) { - if (c->args->args[i].value.integer >= 0) { - gpr_backoff_init( - &c->backoff_state, GRPC_SUBCHANNEL_RECONNECT_BACKOFF_MULTIPLIER, - GRPC_SUBCHANNEL_RECONNECT_JITTER, - GPR_MIN(c->args->args[i].value.integer, - GRPC_SUBCHANNEL_INITIAL_CONNECT_BACKOFF_SECONDS * 1000), - c->args->args[i].value.integer); - } else { - gpr_log(GPR_ERROR, GRPC_ARG_MAX_RECONNECT_BACKOFF_MS - " : must be non-negative"); - } - } else { - gpr_log(GPR_ERROR, - GRPC_ARG_MAX_RECONNECT_BACKOFF_MS " : must be an integer"); - } + fixed_reconnect_backoff = true; + initial_backoff_ms = max_backoff_ms = grpc_channel_arg_get_integer( + &c->args->args[i], + (grpc_integer_options){initial_backoff_ms, 100, INT_MAX}); + } else if (0 == strcmp(c->args->args[i].key, + GRPC_ARG_MAX_RECONNECT_BACKOFF_MS)) { + fixed_reconnect_backoff = false; + max_backoff_ms = grpc_channel_arg_get_integer( + &c->args->args[i], + (grpc_integer_options){max_backoff_ms, 100, INT_MAX}); + } else if (0 == strcmp(c->args->args[i].key, + GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS)) { + fixed_reconnect_backoff = false; + max_backoff_ms = grpc_channel_arg_get_integer( + &c->args->args[i], + (grpc_integer_options){initial_backoff_ms, 100, INT_MAX}); } } } + gpr_backoff_init( + &c->backoff_state, + fixed_reconnect_backoff ? 1.0 + : GRPC_SUBCHANNEL_RECONNECT_BACKOFF_MULTIPLIER, + fixed_reconnect_backoff ? 0.0 : GRPC_SUBCHANNEL_RECONNECT_JITTER, + initial_backoff_ms, max_backoff_ms); gpr_mu_init(&c->mu); return grpc_subchannel_index_register(exec_ctx, key, c); From 4e5a0163e01eb19ca21427b93392b7fd8e314693 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 28 Sep 2016 15:39:55 -0700 Subject: [PATCH 2/5] Fix copy/paste bug --- src/core/ext/client_config/subchannel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index ebf13109586..646e6027f83 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -353,7 +353,7 @@ grpc_subchannel *grpc_subchannel_create(grpc_exec_ctx *exec_ctx, } else if (0 == strcmp(c->args->args[i].key, GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS)) { fixed_reconnect_backoff = false; - max_backoff_ms = grpc_channel_arg_get_integer( + initial_backoff_ms = grpc_channel_arg_get_integer( &c->args->args[i], (grpc_integer_options){initial_backoff_ms, 100, INT_MAX}); } From 45c8c30ad38d32ef6b5c60d9023e56deea9b84ef Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 28 Sep 2016 17:19:48 -0700 Subject: [PATCH 3/5] Set initial backoff to 10000ms when creating channel --- src/objective-c/GRPCClient/private/GRPCHost.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/objective-c/GRPCClient/private/GRPCHost.m b/src/objective-c/GRPCClient/private/GRPCHost.m index 9cd9593d172..57ecf092fe4 100644 --- a/src/objective-c/GRPCClient/private/GRPCHost.m +++ b/src/objective-c/GRPCClient/private/GRPCHost.m @@ -219,6 +219,8 @@ static NSMutableDictionary *kHostCache; if (_responseSizeLimitOverride) { args[@GRPC_ARG_MAX_MESSAGE_LENGTH] = _responseSizeLimitOverride; } + NSNumber *initialBackoff = [NSNumber numberWithInt:10000]; + args[@GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS] = initialBackoff; return args; } From 31c3e01ce9717ce4d3245abd700ac1194ae6c275 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 29 Sep 2016 11:26:12 -0700 Subject: [PATCH 4/5] Clean up code --- src/objective-c/GRPCClient/private/GRPCHost.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/objective-c/GRPCClient/private/GRPCHost.m b/src/objective-c/GRPCClient/private/GRPCHost.m index 57ecf092fe4..b57c54b6cea 100644 --- a/src/objective-c/GRPCClient/private/GRPCHost.m +++ b/src/objective-c/GRPCClient/private/GRPCHost.m @@ -219,8 +219,8 @@ static NSMutableDictionary *kHostCache; if (_responseSizeLimitOverride) { args[@GRPC_ARG_MAX_MESSAGE_LENGTH] = _responseSizeLimitOverride; } - NSNumber *initialBackoff = [NSNumber numberWithInt:10000]; - args[@GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS] = initialBackoff; + // Use 10000ms initial backoff time for correct behavior on bad/slow networks + args[@GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS] = @10000; return args; } From 877caede868cb19978f745bc78236ef5bed8d2e6 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 17 Oct 2016 15:16:48 -0700 Subject: [PATCH 5/5] Backport the grpc_channel_arg_get_integer function --- src/core/ext/client_config/subchannel.c | 1 + src/core/lib/channel/channel_args.c | 18 ++++++++++++++++++ src/core/lib/channel/channel_args.h | 8 ++++++++ 3 files changed, 27 insertions(+) diff --git a/src/core/ext/client_config/subchannel.c b/src/core/ext/client_config/subchannel.c index 646e6027f83..02cdf8648e0 100644 --- a/src/core/ext/client_config/subchannel.c +++ b/src/core/ext/client_config/subchannel.c @@ -33,6 +33,7 @@ #include "src/core/ext/client_config/subchannel.h" +#include #include #include diff --git a/src/core/lib/channel/channel_args.c b/src/core/lib/channel/channel_args.c index 79ceeb66b38..3a56b1ff205 100644 --- a/src/core/lib/channel/channel_args.c +++ b/src/core/lib/channel/channel_args.c @@ -271,3 +271,21 @@ int grpc_channel_args_compare(const grpc_channel_args *a, } return 0; } + +int grpc_channel_arg_get_integer(grpc_arg *arg, grpc_integer_options options) { + if (arg->type != GRPC_ARG_INTEGER) { + gpr_log(GPR_ERROR, "%s ignored: it must be an integer", arg->key); + return options.default_value; + } + if (arg->value.integer < options.min_value) { + gpr_log(GPR_ERROR, "%s ignored: it must be >= %d", arg->key, + options.min_value); + return options.default_value; + } + if (arg->value.integer > options.max_value) { + gpr_log(GPR_ERROR, "%s ignored: it must be <= %d", arg->key, + options.max_value); + return options.default_value; + } + return arg->value.integer; +} diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h index 653d04f4279..e85e979eeb7 100644 --- a/src/core/lib/channel/channel_args.h +++ b/src/core/lib/channel/channel_args.h @@ -87,4 +87,12 @@ uint32_t grpc_channel_args_compression_algorithm_get_states( int grpc_channel_args_compare(const grpc_channel_args *a, const grpc_channel_args *b); +typedef struct grpc_integer_options { + int default_value; // Return this if value is outside of expected bounds. + int min_value; + int max_value; +} grpc_integer_options; +/** Returns the value of \a arg, subject to the contraints in \a options. */ +int grpc_channel_arg_get_integer(grpc_arg *arg, grpc_integer_options options); + #endif /* GRPC_CORE_LIB_CHANNEL_CHANNEL_ARGS_H */