From 8d7e32961a2d5563fb2b9dd7b8435ab24c02e373 Mon Sep 17 00:00:00 2001 From: Richard Belleville Date: Mon, 19 Aug 2024 22:04:39 +0000 Subject: [PATCH] Change flag name and invert the bool --- src/core/lib/config/config_vars.cc | 16 ++++++++++------ src/core/lib/config/config_vars.h | 12 ++++++++---- src/core/lib/config/config_vars.yaml | 6 +++--- src/cpp/ext/proto_server_reflection_plugin.cc | 6 +++--- tools/codegen/core/gen_config_vars.py | 4 +++- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/core/lib/config/config_vars.cc b/src/core/lib/config/config_vars.cc index 98c8d672dd6fb..e0db62f5d0979 100644 --- a/src/core/lib/config/config_vars.cc +++ b/src/core/lib/config/config_vars.cc @@ -72,8 +72,10 @@ ABSL_FLAG(absl::optional, grpc_not_use_system_ssl_roots, {}, "Disable loading system root certificates."); ABSL_FLAG(absl::optional, grpc_ssl_cipher_suites, {}, "A colon separated list of cipher suites to use with OpenSSL"); -ABSL_FLAG(absl::optional, grpc_cpp_enable_reflection, {}, - "Whether to automatically add a reflection handler."); +ABSL_FLAG(absl::optional, grpc_cpp_experimental_disable_reflection, {}, + "EXPERIMENTAL. Only respected when there is a dependency on " + ":grpc++_reflection. If true, no reflection server will be " + "automatically added."); namespace grpc_core { @@ -91,9 +93,10 @@ ConfigVars::ConfigVars(const Overrides& overrides) not_use_system_ssl_roots_(LoadConfig( FLAGS_grpc_not_use_system_ssl_roots, "GRPC_NOT_USE_SYSTEM_SSL_ROOTS", overrides.not_use_system_ssl_roots, false)), - cpp_enable_reflection_(LoadConfig(FLAGS_grpc_cpp_enable_reflection, - "GRPC_CPP_ENABLE_REFLECTION", - overrides.cpp_enable_reflection, true)), + cpp_experimental_disable_reflection_( + LoadConfig(FLAGS_grpc_cpp_experimental_disable_reflection, + "GRPC_CPP_EXPERIMENTAL_DISABLE_REFLECTION", + overrides.cpp_experimental_disable_reflection, false)), dns_resolver_(LoadConfig(FLAGS_grpc_dns_resolver, "GRPC_DNS_RESOLVER", overrides.dns_resolver, "")), verbosity_(LoadConfig(FLAGS_grpc_verbosity, "GRPC_VERBOSITY", @@ -142,7 +145,8 @@ std::string ConfigVars::ToString() const { absl::CEscape(DefaultSslRootsFilePath()), "\"", ", not_use_system_ssl_roots: ", NotUseSystemSslRoots() ? "true" : "false", ", ssl_cipher_suites: ", "\"", absl::CEscape(SslCipherSuites()), "\"", - ", cpp_enable_reflection: ", CppEnableReflection() ? "true" : "false"); + ", cpp_experimental_disable_reflection: ", + CppExperimentalDisableReflection() ? "true" : "false"); } } // namespace grpc_core diff --git a/src/core/lib/config/config_vars.h b/src/core/lib/config/config_vars.h index aa7eeeffeacdb..32220cd1f173f 100644 --- a/src/core/lib/config/config_vars.h +++ b/src/core/lib/config/config_vars.h @@ -38,7 +38,7 @@ class GPR_DLL ConfigVars { absl::optional enable_fork_support; absl::optional abort_on_leaks; absl::optional not_use_system_ssl_roots; - absl::optional cpp_enable_reflection; + absl::optional cpp_experimental_disable_reflection; absl::optional dns_resolver; absl::optional verbosity; absl::optional poll_strategy; @@ -98,8 +98,12 @@ class GPR_DLL ConfigVars { bool NotUseSystemSslRoots() const { return not_use_system_ssl_roots_; } // A colon separated list of cipher suites to use with OpenSSL absl::string_view SslCipherSuites() const { return ssl_cipher_suites_; } - // Whether to automatically add a reflection handler. - bool CppEnableReflection() const { return cpp_enable_reflection_; } + // EXPERIMENTAL. Only respected when there is a dependency on + // :grpc++_reflection. If true, no reflection server will be automatically + // added. + bool CppExperimentalDisableReflection() const { + return cpp_experimental_disable_reflection_; + } private: explicit ConfigVars(const Overrides& overrides); @@ -109,7 +113,7 @@ class GPR_DLL ConfigVars { bool enable_fork_support_; bool abort_on_leaks_; bool not_use_system_ssl_roots_; - bool cpp_enable_reflection_; + bool cpp_experimental_disable_reflection_; std::string dns_resolver_; std::string verbosity_; std::string poll_strategy_; diff --git a/src/core/lib/config/config_vars.yaml b/src/core/lib/config/config_vars.yaml index 68b8e8e228bd9..7246d8ba8ba14 100644 --- a/src/core/lib/config/config_vars.yaml +++ b/src/core/lib/config/config_vars.yaml @@ -120,7 +120,7 @@ ECDHE-ECDSA-AES256-GCM-SHA384:\ ECDHE-RSA-AES128-GCM-SHA256:\ ECDHE-RSA-AES256-GCM-SHA384" -- name: cpp_enable_reflection +- name: cpp_experimental_disable_reflection type: bool - description: Whether to automatically add a reflection handler. - default: true + description: "EXPERIMENTAL. Only respected when there is a dependency on :grpc++_reflection. If true, no reflection server will be automatically added." + default: false diff --git a/src/cpp/ext/proto_server_reflection_plugin.cc b/src/cpp/ext/proto_server_reflection_plugin.cc index d37be565613ae..4299884055df6 100644 --- a/src/cpp/ext/proto_server_reflection_plugin.cc +++ b/src/cpp/ext/proto_server_reflection_plugin.cc @@ -45,7 +45,7 @@ void ProtoServerReflectionPlugin::InitServer(grpc::ServerInitializer* si) { // We cannot simply keep the plugin from being unregistered because this must // happen at static initialization time, whereas flag configuration that // controls this is not received until later. - if (grpc_core::ConfigVars::Get().CppEnableReflection()) { + if (!grpc_core::ConfigVars::Get().CppExperimentalDisableReflection()) { si->RegisterService(reflection_service_v1_); si->RegisterService(reflection_service_v1alpha_); } @@ -59,7 +59,7 @@ void ProtoServerReflectionPlugin::ChangeArguments(const std::string& /*name*/, void* /*value*/) {} bool ProtoServerReflectionPlugin::has_sync_methods() const { - if (grpc_core::ConfigVars::Get().CppEnableReflection()) { + if (!grpc_core::ConfigVars::Get().CppExperimentalDisableReflection()) { return (reflection_service_v1_ && reflection_service_v1_->has_synchronous_methods()) || (reflection_service_v1alpha_ && @@ -69,7 +69,7 @@ bool ProtoServerReflectionPlugin::has_sync_methods() const { } bool ProtoServerReflectionPlugin::has_async_methods() const { - if (grpc_core::ConfigVars::Get().CppEnableReflection()) { + if (!grpc_core::ConfigVars::Get().CppExperimentalDisableReflection()) { return (reflection_service_v1_ && reflection_service_v1_->has_async_methods()) || (reflection_service_v1alpha_ && diff --git a/tools/codegen/core/gen_config_vars.py b/tools/codegen/core/gen_config_vars.py index 1aa217a3ee382..036a59518bc47 100755 --- a/tools/codegen/core/gen_config_vars.py +++ b/tools/codegen/core/gen_config_vars.py @@ -39,7 +39,9 @@ print("config has no name: %r" % attr) error = True continue - if "experiment" in attr["name"] and attr["name"] != "experiments": + if ( + "experiment" in attr["name"] and "experimental" not in attr["name"] + ) and attr["name"] != "experiments": print("use experiment system for experiments") error = True if "description" not in attr: