From 7b2484b380a061626010467efe5c24db83dc3db6 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 5 Oct 2017 15:31:43 +0000 Subject: Document env var --- doc/environment_variables.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'doc') diff --git a/doc/environment_variables.md b/doc/environment_variables.md index f775de1664..c5fb266498 100644 --- a/doc/environment_variables.md +++ b/doc/environment_variables.md @@ -49,6 +49,7 @@ some configuration as environment variables that can be set. - connectivity_state - traces connectivity state changes to channels - channel_stack_builder - traces information about channel stacks being built - executor - traces grpc's internal thread pool ('the executor') + - glb - traces the grpclb load balancer - http - traces state in the http2 transport engine - http2_stream_state - traces all http2 stream state mutations. - http1 - traces HTTP/1.x operations performed by gRPC @@ -56,11 +57,12 @@ some configuration as environment variables that can be set. - flowctl - traces http2 flow control - op_failure - traces error information when failure is pushed onto a completion queue - - round_robin - traces the round_robin load balancing policy - pick_first - traces the pick first load balancing policy - plugin_credentials - traces plugin credentials + - pollable_refcount - traces reference counting of 'pollable' objects (only + in DEBUG) - resource_quota - trace resource quota objects internals - - glb - traces the grpclb load balancer + - round_robin - traces the round_robin load balancing policy - queue_pluck - queue_timeout - server_channel - lightweight trace of significant server channel events -- cgit v1.2.3 From cd42eb0a8ea21e3002f4377b24d5f54ae7791833 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Fri, 6 Oct 2017 15:26:00 -0700 Subject: Doc with plans for converting core to C++ --- doc/core/moving-to-c++.md | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 doc/core/moving-to-c++.md (limited to 'doc') diff --git a/doc/core/moving-to-c++.md b/doc/core/moving-to-c++.md new file mode 100644 index 0000000000..33c3cfa0e6 --- /dev/null +++ b/doc/core/moving-to-c++.md @@ -0,0 +1,54 @@ +# Moving gRPC core to C++ + +October 2017 + +ctiller, markdroth, vjpai + +## Background and Goal + +gRPC core was originally written in C89 for several reasons (possibility of +kernel integration, ease of wrapping, compiler support, etc). Over time, this +was changed to C99 as all relevant compilers in active use came to support C99 +effectively. Now, gRPC core is C++ (although the code is still idiomatically C +code) with C linkage for public functions. Throughout all of these transitions, +the public header files are committed to remain in C89. + +The goal now is to make gRPC core true idiomatic C++ compatible with +[Google's C++ style guide](https://google.github.io/styleguide/cppguide.html). + +## Constraints + +- No use of standard library + - Standard library makes wrapping difficult/impossible and also reduces platform portability + - This takes precedence over using C++ style guide +- But lambdas are ok +- As are third-party libraries that meet our build requirements (such as many parts of abseil) +- There will be some C++ features that don't work + - `new` and `delete` + - pure virtual functions are not allowed because the message that prints out "Pure Virtual Function called" is part of the standard library + - Make a `#define GRPC_ABSTRACT {GPR_ASSERT(false);}` instead of `= 0;` +- The sanity for making sure that we don't depend on libstdc++ is that at least some tests should explicitly not include it + - Most tests can migrate to use gtest + - There are tremendous # of code paths that can now be exposed to unit tests because of the use of gtest and C++ + - But at least some tests should not use gtest + + +## Roadmap + +- What should be the phases of getting code converted to idiomatic C++ + - Opportunistically do leaf code that other parts don't depend on + - Spend a little time deciding how to do non-leaf stuff that isn't central or polymorphic (e.g., timer, call combiner) + - For big central or polymorphic interfaces, actually do an API review (for things like transport, filter API, endpoint, closure, exec_ctx, ...) . + - Core internal changes don't need a gRFC, but core surface changes do + - But an API review should include at least a PR with the header change and tests to use it before it gets used more broadly + - iomgr polling for POSIX is a gray area whether it's a leaf or central +- What is the schedule? + - In Q4 2017, if some stuff happens opportunistically, great; otherwise ¯\\\_(ツ)\_/¯ + - More updates as team time becomes available and committed to this project + +## Implications for C++ API and wrapped languages + +- For C++ structs, switch to `using` when possible (e.g., Slice, ByteBuffer, ...) +- Can we get wrapped languages to a point where we can statically link C++? This will take a year in probability but that would allow the use of `std::` + - Are there other environments that don't support std library, like maybe Android NDK? + - Probably, that might push things out to 18 months -- cgit v1.2.3 From d137066be8c9527b5cea36132915619cc61cfc6d Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Mon, 9 Oct 2017 10:15:37 -0700 Subject: Add some details --- doc/core/moving-to-c++.md | 24 +++++++++++++++--------- tools/doxygen/Doxyfile.core | 1 + tools/doxygen/Doxyfile.core.internal | 1 + 3 files changed, 17 insertions(+), 9 deletions(-) (limited to 'doc') diff --git a/doc/core/moving-to-c++.md b/doc/core/moving-to-c++.md index 33c3cfa0e6..4c745b38a9 100644 --- a/doc/core/moving-to-c++.md +++ b/doc/core/moving-to-c++.md @@ -6,14 +6,17 @@ ctiller, markdroth, vjpai ## Background and Goal -gRPC core was originally written in C89 for several reasons (possibility of -kernel integration, ease of wrapping, compiler support, etc). Over time, this -was changed to C99 as all relevant compilers in active use came to support C99 -effectively. Now, gRPC core is C++ (although the code is still idiomatically C -code) with C linkage for public functions. Throughout all of these transitions, -the public header files are committed to remain in C89. - -The goal now is to make gRPC core true idiomatic C++ compatible with +gRPC core was originally written in C89 for several reasons +(possibility of kernel integration, ease of wrapping, compiler +support, etc). Over time, this was changed to C99 as all relevant +compilers in active use came to support C99 effectively. +[Now, gRPC core is C++](https://github.com/grpc/proposal/blob/master/L6-allow-c%2B%2B-in-grpc-core.md) +(although the code is still idiomatically C code) with C linkage for +public functions. Throughout all of these transitions, the public +header files are committed to remain in C89. + +The goal now is to make the gRPC core implementation true idiomatic +C++ compatible with [Google's C++ style guide](https://google.github.io/styleguide/cppguide.html). ## Constraints @@ -48,7 +51,10 @@ The goal now is to make gRPC core true idiomatic C++ compatible with ## Implications for C++ API and wrapped languages -- For C++ structs, switch to `using` when possible (e.g., Slice, ByteBuffer, ...) +- For C++ structs, switch to `using` when possible (e.g., Slice, +ByteBuffer, ...) +- The C++ API implementation might directly start using +`grpc_transport_stream_op_batch` rather than the core surface `grpc_op`. - Can we get wrapped languages to a point where we can statically link C++? This will take a year in probability but that would allow the use of `std::` - Are there other environments that don't support std library, like maybe Android NDK? - Probably, that might push things out to 18 months diff --git a/tools/doxygen/Doxyfile.core b/tools/doxygen/Doxyfile.core index b8514fe311..c8fd2ee48b 100644 --- a/tools/doxygen/Doxyfile.core +++ b/tools/doxygen/Doxyfile.core @@ -772,6 +772,7 @@ doc/connection-backoff-interop-test-description.md \ doc/connection-backoff.md \ doc/connectivity-semantics-and-api.md \ doc/core/grpc-error.md \ +doc/core/moving-to-c++.md \ doc/core/pending_api_cleanups.md \ doc/cpp-style-guide.md \ doc/environment_variables.md \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index ee593e3ea0..3047778737 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -772,6 +772,7 @@ doc/connection-backoff-interop-test-description.md \ doc/connection-backoff.md \ doc/connectivity-semantics-and-api.md \ doc/core/grpc-error.md \ +doc/core/moving-to-c++.md \ doc/core/pending_api_cleanups.md \ doc/cpp-style-guide.md \ doc/environment_variables.md \ -- cgit v1.2.3 From 9f02a27ceb0777951f84b8470a1409b20c9f2d28 Mon Sep 17 00:00:00 2001 From: Juanli Shen Date: Tue, 10 Oct 2017 12:57:01 -0700 Subject: Update load-balancing.md --- doc/load-balancing.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'doc') diff --git a/doc/load-balancing.md b/doc/load-balancing.md index 88ff35496f..401900af88 100644 --- a/doc/load-balancing.md +++ b/doc/load-balancing.md @@ -129,10 +129,9 @@ works: by the resolver. It asks the balancer for the server addresses to use for the server name originally requested by the client (i.e., the same one originally passed to the name resolver). - - Note: The `grpclb` policy currently ignores any non-balancer - addresses returned by the resolver. However, in the future, it - may be changed to use these addresses as a fallback in case no - balancers can be contacted. + - Note: In `grpclb` policy, the non-balancer addresses returned by + the resolver are used as a fallback in case no balancers can be + contacted. 2. The gRPC servers to which the load balancer is directing the client may report load to the load balancers, if that information is needed by the load balancer's configuration. -- cgit v1.2.3 From df7583e0b6f715ed06eeeda4a3f2bfc4b6940e22 Mon Sep 17 00:00:00 2001 From: Juanli Shen Date: Wed, 11 Oct 2017 09:26:12 -0700 Subject: Update load-balancing.md --- doc/load-balancing.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'doc') diff --git a/doc/load-balancing.md b/doc/load-balancing.md index 401900af88..8ff94075b5 100644 --- a/doc/load-balancing.md +++ b/doc/load-balancing.md @@ -129,9 +129,9 @@ works: by the resolver. It asks the balancer for the server addresses to use for the server name originally requested by the client (i.e., the same one originally passed to the name resolver). - - Note: In `grpclb` policy, the non-balancer addresses returned by - the resolver are used as a fallback in case no balancers can be - contacted. + - Note: In the `grpclb` policy, the non-balancer addresses returned + by the resolver are used as a fallback in case no balancers can be + contacted when the LB policy is started. 2. The gRPC servers to which the load balancer is directing the client may report load to the load balancers, if that information is needed by the load balancer's configuration. -- cgit v1.2.3 From b41014eeefa682c80d8bf48c720b23d00de29e03 Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Thu, 5 Oct 2017 23:00:06 -0700 Subject: Add GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS --- doc/environment_variables.md | 14 ++++---- .../ext/filters/client_channel/backup_poller.cc | 38 +++++++++++++--------- 2 files changed, 29 insertions(+), 23 deletions(-) (limited to 'doc') diff --git a/doc/environment_variables.md b/doc/environment_variables.md index c5fb266498..0163235d63 100644 --- a/doc/environment_variables.md +++ b/doc/environment_variables.md @@ -120,10 +120,10 @@ some configuration as environment variables that can be set. perform name resolution - ares - a DNS resolver based around the c-ares library -* GRPC_DISABLE_CHANNEL_CONNECTIVITY_WATCHER - The channel connectivity watcher uses one extra thread to check the channel - state every 500 ms on the client side. It can help reconnect disconnected - client channels (mostly due to idleness), so that the next RPC on this channel - won't fail. Set to 1 to turn off this watcher and save a thread. Please note - this is a temporary work-around, it will be removed in the future once we have - support for automatically reestablishing failed connections. +* GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS + Default: 500 + Declares the interval between two backup polls on client channels. These polls + are run in the timer thread so that gRPC can process connection failures while + there is no active polling thread. They help reconnect disconnected client + channels (mostly due to idleness), so that the next RPC on this channel won't + fail. Set to 0 to turn off the backup polls. diff --git a/src/core/ext/filters/client_channel/backup_poller.cc b/src/core/ext/filters/client_channel/backup_poller.cc index 36b55ebf9a..784e75624b 100644 --- a/src/core/ext/filters/client_channel/backup_poller.cc +++ b/src/core/ext/filters/client_channel/backup_poller.cc @@ -31,7 +31,7 @@ #include "src/core/lib/surface/channel.h" #include "src/core/lib/surface/completion_queue.h" -#define DEFAULT_POLLING_INTERVAL_MS 500 +#define DEFAULT_POLL_INTERVAL_MS 500 typedef struct backup_poller { grpc_timer polling_timer; @@ -46,14 +46,18 @@ typedef struct backup_poller { static gpr_once g_once = GPR_ONCE_INIT; static gpr_mu g_poller_mu; static backup_poller* g_poller = NULL; +static int g_poll_interval_ms = DEFAULT_POLL_INTERVAL_MS; -static void init_g_poller_mu() { gpr_mu_init(&g_poller_mu); } - -static bool is_disabled() { - char* env = gpr_getenv("GRPC_DISABLE_CHANNEL_backup_poller"); - bool disabled = gpr_is_true(env); +static void init_g_poller_mu() { + gpr_mu_init(&g_poller_mu); + char* env = gpr_getenv("GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS"); + if (env != NULL) { + int poll_interval_ms = gpr_parse_nonnegative_int(env); + if (poll_interval_ms != -1) { + g_poll_interval_ms = poll_interval_ms; + } + } gpr_free(env); - return disabled; } static bool backup_poller_shutdown_unref(grpc_exec_ctx* exec_ctx, @@ -103,14 +107,15 @@ static void run_poller(grpc_exec_ctx* exec_ctx, void* arg, grpc_error* error) { GRPC_LOG_IF_ERROR("Run client channel backup poller", err); grpc_timer_init( exec_ctx, &p->polling_timer, - gpr_time_add( - now, gpr_time_from_millis(DEFAULT_POLLING_INTERVAL_MS, GPR_TIMESPAN)), + gpr_time_add(now, gpr_time_from_millis(g_poll_interval_ms, GPR_TIMESPAN)), &p->run_poller_closure, now); } void grpc_client_channel_start_backup_polling( grpc_exec_ctx* exec_ctx, grpc_pollset_set* interested_parties) { - if (is_disabled()) return; + if (g_poll_interval_ms == 0) { + return; + } gpr_once_init(&g_once, init_g_poller_mu); gpr_mu_lock(&g_poller_mu); if (g_poller == NULL) { @@ -123,11 +128,10 @@ void grpc_client_channel_start_backup_polling( GRPC_CLOSURE_INIT(&g_poller->run_poller_closure, run_poller, g_poller, grpc_schedule_on_exec_ctx); gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); - grpc_timer_init( - exec_ctx, &g_poller->polling_timer, - gpr_time_add(now, gpr_time_from_millis(DEFAULT_POLLING_INTERVAL_MS, - GPR_TIMESPAN)), - &g_poller->run_poller_closure, now); + grpc_timer_init(exec_ctx, &g_poller->polling_timer, + gpr_time_add(now, gpr_time_from_millis(g_poll_interval_ms, + GPR_TIMESPAN)), + &g_poller->run_poller_closure, now); } gpr_ref(&g_poller->refs); gpr_mu_unlock(&g_poller_mu); @@ -137,7 +141,9 @@ void grpc_client_channel_start_backup_polling( void grpc_client_channel_stop_backup_polling( grpc_exec_ctx* exec_ctx, grpc_pollset_set* interested_parties) { - if (is_disabled()) return; + if (g_poll_interval_ms == 0) { + return; + } grpc_pollset_set_del_pollset(exec_ctx, interested_parties, g_poller->pollset); g_poller_unref(exec_ctx); } -- cgit v1.2.3 From cbb9296b729a312ca9ea59ef53cf022994c31dff Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Wed, 18 Oct 2017 11:47:10 -0700 Subject: Change the default interval to 5 seconds --- doc/environment_variables.md | 2 +- src/core/ext/filters/client_channel/backup_poller.cc | 2 +- test/cpp/end2end/async_end2end_test.cc | 7 ++++--- test/cpp/end2end/end2end_test.cc | 7 ++++--- 4 files changed, 10 insertions(+), 8 deletions(-) (limited to 'doc') diff --git a/doc/environment_variables.md b/doc/environment_variables.md index 0163235d63..40af758f69 100644 --- a/doc/environment_variables.md +++ b/doc/environment_variables.md @@ -121,7 +121,7 @@ some configuration as environment variables that can be set. - ares - a DNS resolver based around the c-ares library * GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS - Default: 500 + Default: 5000 Declares the interval between two backup polls on client channels. These polls are run in the timer thread so that gRPC can process connection failures while there is no active polling thread. They help reconnect disconnected client diff --git a/src/core/ext/filters/client_channel/backup_poller.cc b/src/core/ext/filters/client_channel/backup_poller.cc index dc1f87bc6c..e717c4eb7e 100644 --- a/src/core/ext/filters/client_channel/backup_poller.cc +++ b/src/core/ext/filters/client_channel/backup_poller.cc @@ -31,7 +31,7 @@ #include "src/core/lib/surface/channel.h" #include "src/core/lib/surface/completion_queue.h" -#define DEFAULT_POLL_INTERVAL_MS 500 +#define DEFAULT_POLL_INTERVAL_MS 5000 typedef struct backup_poller { grpc_timer polling_timer; diff --git a/test/cpp/end2end/async_end2end_test.cc b/test/cpp/end2end/async_end2end_test.cc index f8ad4c333c..af3bdb25ac 100644 --- a/test/cpp/end2end/async_end2end_test.cc +++ b/test/cpp/end2end/async_end2end_test.cc @@ -461,6 +461,7 @@ TEST_P(AsyncEnd2endTest, ReconnectChannel) { if (GetParam().inproc) { return; } + gpr_setenv("GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS", "200"); int poller_slowdown_factor = 1; // It needs 2 pollset_works to reconnect the channel with polling engine // "poll" @@ -478,12 +479,12 @@ TEST_P(AsyncEnd2endTest, ReconnectChannel) { while (cq_->Next(&ignored_tag, &ignored_ok)) ; BuildAndStartServer(); - // It needs more than kConnectivityCheckIntervalMsec time to reconnect the - // channel. + // It needs more than GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS time to + // reconnect the channel. gpr_sleep_until(gpr_time_add( gpr_now(GPR_CLOCK_REALTIME), gpr_time_from_millis( - 600 * poller_slowdown_factor * grpc_test_slowdown_factor(), + 300 * poller_slowdown_factor * grpc_test_slowdown_factor(), GPR_TIMESPAN))); SendRpc(1); } diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 2272c9945e..82ca39466e 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -706,6 +706,7 @@ TEST_P(End2endTest, ReconnectChannel) { if (GetParam().inproc) { return; } + gpr_setenv("GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS", "200"); int poller_slowdown_factor = 1; // It needs 2 pollset_works to reconnect the channel with polling engine // "poll" @@ -717,12 +718,12 @@ TEST_P(End2endTest, ReconnectChannel) { ResetStub(); SendRpc(stub_.get(), 1, false); RestartServer(std::shared_ptr()); - // It needs more than kConnectivityCheckIntervalMsec time to reconnect the - // channel. + // It needs more than GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS time to + // reconnect the channel. gpr_sleep_until(gpr_time_add( gpr_now(GPR_CLOCK_REALTIME), gpr_time_from_millis( - 600 * poller_slowdown_factor * grpc_test_slowdown_factor(), + 300 * poller_slowdown_factor * grpc_test_slowdown_factor(), GPR_TIMESPAN))); SendRpc(stub_.get(), 1, false); } -- cgit v1.2.3