From afb982981944a4c1a4febece03315aea95192e6a Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 28 Jun 2018 18:04:24 -0700 Subject: Fix the muddled linkeage of channelz --- .../client_channel/client_channel_channelz.cc | 62 ++++++++++++++++++++++ .../client_channel/client_channel_channelz.h | 55 +++++++++++++++++++ .../chttp2/client/insecure/channel_create.cc | 10 ++-- .../chttp2/client/secure/secure_channel_create.cc | 4 +- src/core/lib/channel/channelz.cc | 37 ++++++------- src/core/lib/channel/channelz.h | 27 ++++++++-- src/core/lib/surface/channel.cc | 14 ++++- src/python/grpcio/grpc_core_dependencies.py | 1 + 8 files changed, 181 insertions(+), 29 deletions(-) create mode 100644 src/core/ext/filters/client_channel/client_channel_channelz.cc create mode 100644 src/core/ext/filters/client_channel/client_channel_channelz.h (limited to 'src') diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc new file mode 100644 index 0000000000..df78dd5932 --- /dev/null +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -0,0 +1,62 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include + +#include "src/core/ext/filters/client_channel/client_channel_channelz.h" +#include "src/core/lib/gpr/useful.h" + +namespace grpc_core { +namespace channelz { + +static void* client_channel_channelz_copy(void* p) { return p; } + +static void client_channel_channelz_destroy(void* p) {} + +static int client_channel_channelz_cmp(void* a, void* b) { + return GPR_ICMP(a, b); +} + +static const grpc_arg_pointer_vtable client_channel_channelz_vtable = { + client_channel_channelz_copy, client_channel_channelz_destroy, + client_channel_channelz_cmp}; + +bool ClientChannelNode::GetConnectivityState(grpc_connectivity_state* state) { + if (channel()) { + *state = grpc_channel_check_connectivity_state(channel(), false); + } else { + *state = GRPC_CHANNEL_SHUTDOWN; + } + return true; +} + +grpc_arg ClientChannelNode::CreateArg() { + return grpc_channel_arg_pointer_create( + const_cast(GRPC_ARG_CHANNELZ_CHANNEL_NODE_CREATION_FUNC), + reinterpret_cast(MakeClientChannelNode), + &client_channel_channelz_vtable); +} + +RefCountedPtr MakeClientChannelNode( + grpc_channel* channel, size_t channel_tracer_max_nodes) { + return RefCountedPtr( + New(channel, channel_tracer_max_nodes)); +} + +} // namespace channelz +} // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h new file mode 100644 index 0000000000..7d05cda7da --- /dev/null +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -0,0 +1,55 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_CLIENT_CHANNEL_CHANNELZ_H +#define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_CLIENT_CHANNEL_CHANNELZ_H + +#include + +#include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/channel/channelz.h" + +namespace grpc_core { +namespace channelz { + +// Subtype of ChannelNode that overrides and provides client_channel specific +// functionality like querying for connectivity_state and subchannel data. +class ClientChannelNode : public ChannelNode { + public: + ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes) + : ChannelNode(channel, channel_tracer_max_nodes) {} + virtual ~ClientChannelNode() {} + + // Override this functionality since client_channels have a notion of + // channel connectivity. + bool GetConnectivityState(grpc_connectivity_state* state) override; + + // Helper to create a channel arg to ensure this type of ChannelNode is + // created. + static grpc_arg CreateArg(); +}; + +RefCountedPtr MakeClientChannelNode( + grpc_channel* channel, size_t channel_tracer_max_nodes); + +grpc_arg BlahBlah(); + +} // namespace channelz +} // namespace grpc_core + +#endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_CLIENT_CHANNEL_CHANNELZ_H */ diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.cc b/src/core/ext/transport/chttp2/client/insecure/channel_create.cc index e6c8c38260..420629f6b7 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.cc +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.cc @@ -26,6 +26,7 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/ext/filters/client_channel/resolver_registry.h" #include "src/core/ext/transport/chttp2/client/authority.h" #include "src/core/ext/transport/chttp2/client/chttp2_connector.h" @@ -92,10 +93,11 @@ grpc_channel* grpc_insecure_channel_create(const char* target, "grpc_insecure_channel_create(target=%s, args=%p, reserved=%p)", 3, (target, args, reserved)); GPR_ASSERT(reserved == nullptr); - // Add channel arg containing the client channel factory. - grpc_arg arg = - grpc_client_channel_factory_create_channel_arg(&client_channel_factory); - grpc_channel_args* new_args = grpc_channel_args_copy_and_add(args, &arg, 1); + grpc_arg args_to_add[] = { + grpc_client_channel_factory_create_channel_arg(&client_channel_factory), + grpc_core::channelz::ClientChannelNode::CreateArg()}; + grpc_channel_args* new_args = grpc_channel_args_copy_and_add( + args, args_to_add, GPR_ARRAY_SIZE(args_to_add)); // Create channel. grpc_channel* channel = client_channel_factory_create_channel( &client_channel_factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc index 5ce73a95d7..82691d4e25 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc @@ -26,6 +26,7 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/ext/filters/client_channel/resolver_registry.h" #include "src/core/ext/filters/client_channel/uri_parser.h" #include "src/core/ext/transport/chttp2/client/chttp2_connector.h" @@ -213,7 +214,8 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds, // credentials. grpc_arg args_to_add[] = { grpc_client_channel_factory_create_channel_arg(&client_channel_factory), - grpc_channel_credentials_to_arg(creds)}; + grpc_channel_credentials_to_arg(creds), + grpc_core::channelz::ClientChannelNode::CreateArg()}; grpc_channel_args* new_args = grpc_channel_args_copy_and_add( args, args_to_add, GPR_ARRAY_SIZE(args_to_add)); // Create channel. diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index a49271c3a1..62bc4d4330 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -109,15 +109,8 @@ void ChannelNode::RecordCallStarted() { (gpr_atm)ExecCtx::Get()->Now()); } -grpc_connectivity_state ChannelNode::GetConnectivityState() { - if (channel_ == nullptr) { - return GRPC_CHANNEL_SHUTDOWN; - } else { - // TODO(ncteisen): re-enable this once we have cleaned up all of the - // internal dependency issues. - // return grpc_channel_check_connectivity_state(channel_, false); - return GRPC_CHANNEL_IDLE; - } +bool ChannelNode::GetConnectivityState(grpc_connectivity_state* state) { + return false; } char* ChannelNode::RenderJSON() { @@ -140,15 +133,17 @@ char* ChannelNode::RenderJSON() { json = data; json_iterator = nullptr; // create and fill the connectivity state child. - grpc_connectivity_state connectivity_state = GetConnectivityState(); - json_iterator = grpc_json_create_child(json_iterator, json, "state", nullptr, - GRPC_JSON_OBJECT, false); - json = json_iterator; - grpc_json_create_child(nullptr, json, "state", - grpc_connectivity_state_name(connectivity_state), - GRPC_JSON_STRING, false); - // reset the parent to be the data object. - json = data; + grpc_connectivity_state connectivity_state; + if (GetConnectivityState(&connectivity_state)) { + json_iterator = grpc_json_create_child(json_iterator, json, "state", + nullptr, GRPC_JSON_OBJECT, false); + json = json_iterator; + grpc_json_create_child(nullptr, json, "state", + grpc_connectivity_state_name(connectivity_state), + GRPC_JSON_STRING, false); + // reset the parent to be the data object. + json = data; + } json_iterator = grpc_json_create_child( json_iterator, json, "target", target_.get(), GRPC_JSON_STRING, false); // fill in the channel trace if applicable @@ -184,5 +179,11 @@ char* ChannelNode::RenderJSON() { return json_str; } +RefCountedPtr MakeChannelNode(grpc_channel* channel, + size_t channel_tracer_max_nodes) { + return MakeRefCounted( + channel, channel_tracer_max_nodes); +} + } // namespace channelz } // namespace grpc_core diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 2aad1e82f4..5c91658dd1 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -31,6 +31,10 @@ #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/json/json.h" +// Channel arg key for client channel factory. +#define GRPC_ARG_CHANNELZ_CHANNEL_NODE_CREATION_FUNC \ + "grpc.channelz_channel_node_creation_func" + namespace grpc_core { namespace channelz { @@ -41,7 +45,7 @@ class ChannelNodePeer; class ChannelNode : public RefCounted { public: ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); - ~ChannelNode(); + virtual ~ChannelNode(); void RecordCallStarted(); void RecordCallFailed() { @@ -53,6 +57,13 @@ class ChannelNode : public RefCounted { char* RenderJSON(); + // helper for getting connectivity state. It is virtual because it allows + // the client_channel code to live in ext/ instead of lib/ + // + // returns true if the channel has a notion of a connectivity state. In that + // case it also sets state to the correct connectivity state. + virtual bool GetConnectivityState(grpc_connectivity_state* state); + ChannelTrace* trace() { return trace_.get(); } void set_channel_destroyed() { @@ -62,13 +73,13 @@ class ChannelNode : public RefCounted { intptr_t channel_uuid() { return channel_uuid_; } + protected: + grpc_channel* channel() { return channel_; } + private: // testing peer friend. friend class testing::ChannelNodePeer; - // helper for getting connectivity state. - grpc_connectivity_state GetConnectivityState(); - grpc_channel* channel_ = nullptr; UniquePtr target_; gpr_atm calls_started_ = 0; @@ -79,6 +90,14 @@ class ChannelNode : public RefCounted { ManualConstructor trace_; }; +// Creation functions + +typedef RefCountedPtr (*ChannelNodeCreationFunc)(grpc_channel*, + size_t); + +RefCountedPtr MakeChannelNode(grpc_channel* channel, + size_t channel_tracer_max_nodes); + } // namespace channelz } // namespace grpc_core diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index d5d75fcb2a..a17fde015f 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -105,6 +105,10 @@ grpc_channel* grpc_channel_create_with_builder( channel->is_client = grpc_channel_stack_type_is_client(channel_stack_type); size_t channel_tracer_max_nodes = 0; // default to off bool channelz_enabled = false; + // this creates the default ChannelNode. Different types of channels may + // override this to ensure a correct ChannelNode is created. + grpc_core::channelz::ChannelNodeCreationFunc channel_node_create_func = + grpc_core::channelz::MakeChannelNode; gpr_mu_init(&channel->registered_call_mu); channel->registered_calls = nullptr; @@ -145,14 +149,20 @@ grpc_channel* grpc_channel_create_with_builder( (size_t)grpc_channel_arg_get_integer(&args->args[i], options); } else if (0 == strcmp(args->args[i].key, GRPC_ARG_ENABLE_CHANNELZ)) { channelz_enabled = grpc_channel_arg_get_bool(&args->args[i], false); + } else if (0 == strcmp(args->args[i].key, + GRPC_ARG_CHANNELZ_CHANNEL_NODE_CREATION_FUNC)) { + GPR_ASSERT(args->args[i].type == GRPC_ARG_POINTER); + GPR_ASSERT(args->args[i].value.pointer.p != nullptr); + channel_node_create_func = + reinterpret_cast( + args->args[i].value.pointer.p); } } grpc_channel_args_destroy(args); if (channelz_enabled) { channel->channelz_channel = - grpc_core::MakeRefCounted( - channel, channel_tracer_max_nodes); + channel_node_create_func(channel, channel_tracer_max_nodes); channel->channelz_channel->trace()->AddTraceEvent( grpc_core::channelz::ChannelTrace::Severity::Info, grpc_slice_from_static_string("Channel created")); diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 1746020b72..49185cc648 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -306,6 +306,7 @@ CORE_SOURCE_FILES = [ 'src/core/ext/filters/client_channel/backup_poller.cc', 'src/core/ext/filters/client_channel/channel_connectivity.cc', 'src/core/ext/filters/client_channel/client_channel.cc', + 'src/core/ext/filters/client_channel/client_channel_channelz.cc', 'src/core/ext/filters/client_channel/client_channel_factory.cc', 'src/core/ext/filters/client_channel/client_channel_plugin.cc', 'src/core/ext/filters/client_channel/connector.cc', -- cgit v1.2.3 From 230035180fef721ae80ce7c35b574938040f4f80 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 29 Jun 2018 12:00:15 -0700 Subject: Change pattern to have subtype do json population --- .../filters/client_channel/client_channel_channelz.cc | 16 +++++++++++----- .../filters/client_channel/client_channel_channelz.h | 2 +- src/core/lib/channel/channelz.cc | 19 +++---------------- src/core/lib/channel/channelz.h | 10 ++++------ 4 files changed, 19 insertions(+), 28 deletions(-) (limited to 'src') diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index df78dd5932..c9bdce1604 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -20,6 +20,7 @@ #include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/lib/gpr/useful.h" +#include "src/core/lib/transport/connectivity_state.h" namespace grpc_core { namespace channelz { @@ -36,13 +37,18 @@ static const grpc_arg_pointer_vtable client_channel_channelz_vtable = { client_channel_channelz_copy, client_channel_channelz_destroy, client_channel_channelz_cmp}; -bool ClientChannelNode::GetConnectivityState(grpc_connectivity_state* state) { - if (channel()) { - *state = grpc_channel_check_connectivity_state(channel(), false); +void ClientChannelNode::PopulateConnectivityState(grpc_json* json) { + grpc_connectivity_state state; + if (channel() != nullptr) { + state = grpc_channel_check_connectivity_state(channel(), false); } else { - *state = GRPC_CHANNEL_SHUTDOWN; + state = GRPC_CHANNEL_SHUTDOWN; } - return true; + json = grpc_json_create_child(nullptr, json, "state", nullptr, + GRPC_JSON_OBJECT, false); + grpc_json_create_child(nullptr, json, "state", + grpc_connectivity_state_name(state), GRPC_JSON_STRING, + false); } grpc_arg ClientChannelNode::CreateArg() { diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index 7d05cda7da..d339b4e9f6 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -37,7 +37,7 @@ class ClientChannelNode : public ChannelNode { // Override this functionality since client_channels have a notion of // channel connectivity. - bool GetConnectivityState(grpc_connectivity_state* state) override; + void PopulateConnectivityState(grpc_json* json) override; // Helper to create a channel arg to ensure this type of ChannelNode is // created. diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 62bc4d4330..0f13551c2a 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -36,7 +36,6 @@ #include "src/core/lib/iomgr/error.h" #include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/surface/channel.h" -#include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/transport/error_utils.h" namespace grpc_core { @@ -109,9 +108,7 @@ void ChannelNode::RecordCallStarted() { (gpr_atm)ExecCtx::Get()->Now()); } -bool ChannelNode::GetConnectivityState(grpc_connectivity_state* state) { - return false; -} +void ChannelNode::PopulateConnectivityState(grpc_json* json) {} char* ChannelNode::RenderJSON() { // We need to track these three json objects to build our object @@ -132,18 +129,8 @@ char* ChannelNode::RenderJSON() { GRPC_JSON_OBJECT, false); json = data; json_iterator = nullptr; - // create and fill the connectivity state child. - grpc_connectivity_state connectivity_state; - if (GetConnectivityState(&connectivity_state)) { - json_iterator = grpc_json_create_child(json_iterator, json, "state", - nullptr, GRPC_JSON_OBJECT, false); - json = json_iterator; - grpc_json_create_child(nullptr, json, "state", - grpc_connectivity_state_name(connectivity_state), - GRPC_JSON_STRING, false); - // reset the parent to be the data object. - json = data; - } + + PopulateConnectivityState(json); json_iterator = grpc_json_create_child( json_iterator, json, "target", target_.get(), GRPC_JSON_STRING, false); // fill in the channel trace if applicable diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 5c91658dd1..fc972bf802 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -57,12 +57,10 @@ class ChannelNode : public RefCounted { char* RenderJSON(); - // helper for getting connectivity state. It is virtual because it allows - // the client_channel code to live in ext/ instead of lib/ - // - // returns true if the channel has a notion of a connectivity state. In that - // case it also sets state to the correct connectivity state. - virtual bool GetConnectivityState(grpc_connectivity_state* state); + // helper for getting and populating connectivity state. It is virtual + // because it allows the client_channel specific code to live in ext/ + // instead of lib/ + virtual void PopulateConnectivityState(grpc_json* json); ChannelTrace* trace() { return trace_.get(); } -- cgit v1.2.3 From c9c1feffca295ea9b6232c40d6ac09c5b82343d5 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 29 Jun 2018 12:32:45 -0700 Subject: Write ClientChannelNode in terms of client_channel --- .../filters/client_channel/client_channel_channelz.cc | 17 ++++++++++++++--- .../filters/client_channel/client_channel_channelz.h | 6 ++++-- src/core/lib/channel/channelz.h | 7 +++---- src/core/lib/surface/channel.cc | 2 +- 4 files changed, 22 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index c9bdce1604..e9534f2207 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -18,8 +18,10 @@ #include +#include "src/core/ext/filters/client_channel/client_channel.h" #include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/lib/gpr/useful.h" +#include "src/core/lib/surface/channel.h" #include "src/core/lib/transport/connectivity_state.h" namespace grpc_core { @@ -37,12 +39,21 @@ static const grpc_arg_pointer_vtable client_channel_channelz_vtable = { client_channel_channelz_copy, client_channel_channelz_destroy, client_channel_channelz_cmp}; +ClientChannelNode::ClientChannelNode(grpc_channel* channel, + size_t channel_tracer_max_nodes) + : ChannelNode(channel, channel_tracer_max_nodes) { + client_channel_ = + grpc_channel_stack_last_element(grpc_channel_get_channel_stack(channel)); + GPR_ASSERT(client_channel_->filter == &grpc_client_channel_filter); +} + void ClientChannelNode::PopulateConnectivityState(grpc_json* json) { grpc_connectivity_state state; - if (channel() != nullptr) { - state = grpc_channel_check_connectivity_state(channel(), false); - } else { + if (ChannelIsDestroyed()) { state = GRPC_CHANNEL_SHUTDOWN; + } else { + state = + grpc_client_channel_check_connectivity_state(client_channel_, false); } json = grpc_json_create_child(nullptr, json, "state", nullptr, GRPC_JSON_OBJECT, false); diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index d339b4e9f6..a3f8b07fd6 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -31,8 +31,7 @@ namespace channelz { // functionality like querying for connectivity_state and subchannel data. class ClientChannelNode : public ChannelNode { public: - ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes) - : ChannelNode(channel, channel_tracer_max_nodes) {} + ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); virtual ~ClientChannelNode() {} // Override this functionality since client_channels have a notion of @@ -42,6 +41,9 @@ class ClientChannelNode : public ChannelNode { // Helper to create a channel arg to ensure this type of ChannelNode is // created. static grpc_arg CreateArg(); + + private: + grpc_channel_element* client_channel_; }; RefCountedPtr MakeClientChannelNode( diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index fc972bf802..e14e1b73cb 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -64,15 +64,14 @@ class ChannelNode : public RefCounted { ChannelTrace* trace() { return trace_.get(); } - void set_channel_destroyed() { + void MarkChannelDestroyed() { GPR_ASSERT(channel_ != nullptr); channel_ = nullptr; } - intptr_t channel_uuid() { return channel_uuid_; } + bool ChannelIsDestroyed() { return channel_ == nullptr; } - protected: - grpc_channel* channel() { return channel_; } + intptr_t channel_uuid() { return channel_uuid_; } private: // testing peer friend. diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index a17fde015f..71bd24ce95 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -410,7 +410,7 @@ void grpc_channel_internal_unref(grpc_channel* c REF_ARG) { static void destroy_channel(void* arg, grpc_error* error) { grpc_channel* channel = static_cast(arg); if (channel->channelz_channel != nullptr) { - channel->channelz_channel->set_channel_destroyed(); + channel->channelz_channel->MarkChannelDestroyed(); channel->channelz_channel.reset(); } grpc_channel_stack_destroy(CHANNEL_STACK_FROM_CHANNEL(channel)); -- cgit v1.2.3 From caa85b2a4340c54904a41f1c2fc1ffc17e7f8dbb Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 3 Jul 2018 11:25:41 -0700 Subject: Reviewer feedback --- .../filters/client_channel/client_channel_channelz.cc | 19 ++++++++++--------- .../filters/client_channel/client_channel_channelz.h | 17 +++++++++-------- .../filters/client_channel/client_channel_plugin.cc | 9 +++++++++ .../chttp2/client/insecure/channel_create.cc | 10 ++++------ .../chttp2/client/secure/secure_channel_create.cc | 4 +--- src/core/lib/channel/channelz.cc | 5 ++--- src/core/lib/channel/channelz.h | 13 ++++++++----- src/core/lib/gprpp/memory.h | 4 ++-- src/core/lib/gprpp/ref_counted_ptr.h | 5 +++++ src/core/lib/surface/channel.cc | 2 +- 10 files changed, 51 insertions(+), 37 deletions(-) (limited to 'src') diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index e9534f2207..08ceb2dd05 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -26,14 +26,15 @@ namespace grpc_core { namespace channelz { +namespace { -static void* client_channel_channelz_copy(void* p) { return p; } +void* client_channel_channelz_copy(void* p) { return p; } -static void client_channel_channelz_destroy(void* p) {} +void client_channel_channelz_destroy(void* p) {} -static int client_channel_channelz_cmp(void* a, void* b) { - return GPR_ICMP(a, b); -} +int client_channel_channelz_cmp(void* a, void* b) { return GPR_ICMP(a, b); } + +} // namespace static const grpc_arg_pointer_vtable client_channel_channelz_vtable = { client_channel_channelz_copy, client_channel_channelz_destroy, @@ -62,17 +63,17 @@ void ClientChannelNode::PopulateConnectivityState(grpc_json* json) { false); } -grpc_arg ClientChannelNode::CreateArg() { +grpc_arg ClientChannelNode::CreateChannelArg() { return grpc_channel_arg_pointer_create( const_cast(GRPC_ARG_CHANNELZ_CHANNEL_NODE_CREATION_FUNC), reinterpret_cast(MakeClientChannelNode), &client_channel_channelz_vtable); } -RefCountedPtr MakeClientChannelNode( +RefCountedPtr ClientChannelNode::MakeClientChannelNode( grpc_channel* channel, size_t channel_tracer_max_nodes) { - return RefCountedPtr( - New(channel, channel_tracer_max_nodes)); + return MakePolymorphicRefCounted( + channel, channel_tracer_max_nodes); } } // namespace channelz diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index a3f8b07fd6..cf3ef7b6f2 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -31,8 +31,8 @@ namespace channelz { // functionality like querying for connectivity_state and subchannel data. class ClientChannelNode : public ChannelNode { public: - ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); - virtual ~ClientChannelNode() {} + static RefCountedPtr MakeClientChannelNode( + grpc_channel* channel, size_t channel_tracer_max_nodes); // Override this functionality since client_channels have a notion of // channel connectivity. @@ -40,17 +40,18 @@ class ClientChannelNode : public ChannelNode { // Helper to create a channel arg to ensure this type of ChannelNode is // created. - static grpc_arg CreateArg(); + static grpc_arg CreateChannelArg(); + + protected: + GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE + GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW + ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); + virtual ~ClientChannelNode() {} private: grpc_channel_element* client_channel_; }; -RefCountedPtr MakeClientChannelNode( - grpc_channel* channel, size_t channel_tracer_max_nodes); - -grpc_arg BlahBlah(); - } // namespace channelz } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/client_channel_plugin.cc b/src/core/ext/filters/client_channel/client_channel_plugin.cc index 8385852d1b..e0784b7e5c 100644 --- a/src/core/ext/filters/client_channel/client_channel_plugin.cc +++ b/src/core/ext/filters/client_channel/client_channel_plugin.cc @@ -25,6 +25,7 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/ext/filters/client_channel/http_connect_handshaker.h" #include "src/core/ext/filters/client_channel/http_proxy.h" #include "src/core/ext/filters/client_channel/lb_policy_registry.h" @@ -35,6 +36,14 @@ #include "src/core/lib/surface/channel_init.h" static bool append_filter(grpc_channel_stack_builder* builder, void* arg) { + const grpc_channel_args* args = + grpc_channel_stack_builder_get_channel_arguments(builder); + grpc_arg args_to_add[] = { + grpc_core::channelz::ClientChannelNode::CreateChannelArg()}; + grpc_channel_args* new_args = grpc_channel_args_copy_and_add( + args, args_to_add, GPR_ARRAY_SIZE(args_to_add)); + grpc_channel_stack_builder_set_channel_arguments(builder, new_args); + grpc_channel_args_destroy(new_args); return grpc_channel_stack_builder_append_filter( builder, static_cast(arg), nullptr, nullptr); } diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.cc b/src/core/ext/transport/chttp2/client/insecure/channel_create.cc index 420629f6b7..e6c8c38260 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.cc +++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.cc @@ -26,7 +26,6 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" -#include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/ext/filters/client_channel/resolver_registry.h" #include "src/core/ext/transport/chttp2/client/authority.h" #include "src/core/ext/transport/chttp2/client/chttp2_connector.h" @@ -93,11 +92,10 @@ grpc_channel* grpc_insecure_channel_create(const char* target, "grpc_insecure_channel_create(target=%s, args=%p, reserved=%p)", 3, (target, args, reserved)); GPR_ASSERT(reserved == nullptr); - grpc_arg args_to_add[] = { - grpc_client_channel_factory_create_channel_arg(&client_channel_factory), - grpc_core::channelz::ClientChannelNode::CreateArg()}; - grpc_channel_args* new_args = grpc_channel_args_copy_and_add( - args, args_to_add, GPR_ARRAY_SIZE(args_to_add)); + // Add channel arg containing the client channel factory. + grpc_arg arg = + grpc_client_channel_factory_create_channel_arg(&client_channel_factory); + grpc_channel_args* new_args = grpc_channel_args_copy_and_add(args, &arg, 1); // Create channel. grpc_channel* channel = client_channel_factory_create_channel( &client_channel_factory, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc index 82691d4e25..5ce73a95d7 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc @@ -26,7 +26,6 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" -#include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/ext/filters/client_channel/resolver_registry.h" #include "src/core/ext/filters/client_channel/uri_parser.h" #include "src/core/ext/transport/chttp2/client/chttp2_connector.h" @@ -214,8 +213,7 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds, // credentials. grpc_arg args_to_add[] = { grpc_client_channel_factory_create_channel_arg(&client_channel_factory), - grpc_channel_credentials_to_arg(creds), - grpc_core::channelz::ClientChannelNode::CreateArg()}; + grpc_channel_credentials_to_arg(creds)}; grpc_channel_args* new_args = grpc_channel_args_copy_and_add( args, args_to_add, GPR_ARRAY_SIZE(args_to_add)); // Create channel. diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 0f13551c2a..2074cb0cc5 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -129,7 +129,6 @@ char* ChannelNode::RenderJSON() { GRPC_JSON_OBJECT, false); json = data; json_iterator = nullptr; - PopulateConnectivityState(json); json_iterator = grpc_json_create_child( json_iterator, json, "target", target_.get(), GRPC_JSON_STRING, false); @@ -166,8 +165,8 @@ char* ChannelNode::RenderJSON() { return json_str; } -RefCountedPtr MakeChannelNode(grpc_channel* channel, - size_t channel_tracer_max_nodes) { +RefCountedPtr ChannelNode::MakeChannelNode( + grpc_channel* channel, size_t channel_tracer_max_nodes) { return MakeRefCounted( channel, channel_tracer_max_nodes); } diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index e14e1b73cb..9bd01ece50 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -44,8 +44,8 @@ class ChannelNodePeer; class ChannelNode : public RefCounted { public: - ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); - virtual ~ChannelNode(); + static RefCountedPtr MakeChannelNode( + grpc_channel* channel, size_t channel_tracer_max_nodes); void RecordCallStarted(); void RecordCallFailed() { @@ -73,6 +73,12 @@ class ChannelNode : public RefCounted { intptr_t channel_uuid() { return channel_uuid_; } + protected: + GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE + GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW + ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); + virtual ~ChannelNode(); + private: // testing peer friend. friend class testing::ChannelNodePeer; @@ -92,9 +98,6 @@ class ChannelNode : public RefCounted { typedef RefCountedPtr (*ChannelNodeCreationFunc)(grpc_channel*, size_t); -RefCountedPtr MakeChannelNode(grpc_channel* channel, - size_t channel_tracer_max_nodes); - } // namespace channelz } // namespace grpc_core diff --git a/src/core/lib/gprpp/memory.h b/src/core/lib/gprpp/memory.h index 28fcdf1779..e90bedcd9b 100644 --- a/src/core/lib/gprpp/memory.h +++ b/src/core/lib/gprpp/memory.h @@ -31,12 +31,12 @@ // protected destructor. #define GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE \ template \ - friend void Delete(T*); + friend void grpc_core::Delete(T*); // Add this to a class that want to use New(), but has a private or // protected constructor. #define GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW \ template \ - friend T* New(Args&&...); + friend T* grpc_core::New(Args&&...); namespace grpc_core { diff --git a/src/core/lib/gprpp/ref_counted_ptr.h b/src/core/lib/gprpp/ref_counted_ptr.h index 388e2ec410..534d3d03cb 100644 --- a/src/core/lib/gprpp/ref_counted_ptr.h +++ b/src/core/lib/gprpp/ref_counted_ptr.h @@ -107,6 +107,11 @@ inline RefCountedPtr MakeRefCounted(Args&&... args) { return RefCountedPtr(New(std::forward(args)...)); } +template +inline RefCountedPtr MakePolymorphicRefCounted(Args&&... args) { + return RefCountedPtr(New(std::forward(args)...)); +} + } // namespace grpc_core #endif /* GRPC_CORE_LIB_GPRPP_REF_COUNTED_PTR_H */ diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 71bd24ce95..3e4e434a63 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -108,7 +108,7 @@ grpc_channel* grpc_channel_create_with_builder( // this creates the default ChannelNode. Different types of channels may // override this to ensure a correct ChannelNode is created. grpc_core::channelz::ChannelNodeCreationFunc channel_node_create_func = - grpc_core::channelz::MakeChannelNode; + grpc_core::channelz::ChannelNode::MakeChannelNode; gpr_mu_init(&channel->registered_call_mu); channel->registered_calls = nullptr; -- cgit v1.2.3