Skip to content

Commit

Permalink
thrift: refactor build files and imports (#4271)
Browse files Browse the repository at this point in the history 
Refactors build dependencies and imports to make inter-protocol dependencies
easier. Moves the auto-detecting protocol and transports to better-named
files. Splits DecoderEventHandler out of DecoderFilter to allow most
implementors to exclude some filter-specific functions that are not required for
Thrift decoding. Provides an implmentation of DecoderEventHandler that delegates
to another implementation. Generally cleans up some #include directives. The
impetus for this change becomes apparent when introducing a new protocol that
directly responds to a downstream in order to negotiate protocol options.

Risk Level: low, no code changes
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
  • Loading branch information
@zuercher @htuch
zuercher authored and htuch committed Aug 28, 2018
1 parent 5fa8192 commit 68d20b4
Show file tree
Hide file tree
Showing 34 changed files with 782 additions and 651 deletions.
134 changes: 116 additions & 18 deletions source/extensions/filters/network/thrift_proxy/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ envoy_cc_library(
":app_exception_lib",
":conn_manager_lib",
":decoder_lib",
":protocol_lib",
":protocol_interface",
"//include/envoy/registry",
"//source/common/config:utility_lib",
"//source/extensions/filters/network:well_known_names",
Expand Down Expand Up @@ -76,14 +76,24 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "decoder_events_lib",
hdrs = ["decoder_events.h"],
deps = [
":metadata_lib",
":thrift_lib",
],
)

envoy_cc_library(
name = "decoder_lib",
srcs = ["decoder.cc"],
hdrs = ["decoder.h"],
deps = [
":protocol_lib",
":app_exception_lib",
":protocol_interface",
":stats_lib",
":transport_lib",
":transport_interface",
"//source/common/buffer:buffer_lib",
"//source/extensions/filters/network/thrift_proxy/filters:filter_interface",
],
Expand All @@ -105,9 +115,9 @@ envoy_cc_library(
"protocol_converter.h",
],
deps = [
":decoder_events_lib",
":protocol_interface",
"//include/envoy/buffer:buffer_interface",
"//source/extensions/filters/network/thrift_proxy/filters:filter_interface",
],
)

Expand All @@ -118,6 +128,7 @@ envoy_cc_library(
],
external_deps = ["abseil_optional"],
deps = [
":decoder_events_lib",
":metadata_lib",
":thrift_lib",
"//include/envoy/buffer:buffer_interface",
Expand All @@ -129,22 +140,59 @@ envoy_cc_library(
)

envoy_cc_library(
name = "protocol_lib",
name = "auto_protocol_lib",
srcs = [
"auto_protocol_impl.cc",
],
hdrs = [
"auto_protocol_impl.h",
],
deps = [
":binary_protocol_lib",
":buffer_helper_lib",
":compact_protocol_lib",
":protocol_interface",
"//source/common/common:macros",
],
)

envoy_cc_library(
name = "binary_protocol_lib",
srcs = [
"binary_protocol_impl.cc",
"compact_protocol_impl.cc",
"protocol_impl.cc",
],
hdrs = [
"binary_protocol_impl.h",
],
deps = [
":buffer_helper_lib",
":protocol_interface",
"//source/common/common:macros",
],
)

envoy_cc_library(
name = "compact_protocol_lib",
srcs = [
"compact_protocol_impl.cc",
],
hdrs = [
"compact_protocol_impl.h",
"protocol_impl.h",
],
external_deps = ["abseil_optional"],
deps = [
":buffer_helper_lib",
":protocol_interface",
"//source/common/singleton:const_singleton",
"//source/common/common:macros",
],
)

envoy_cc_library(
name = "protocol_lib",
deps = [
":auto_protocol_lib",
":binary_protocol_lib",
":compact_protocol_lib",
],
)

Expand Down Expand Up @@ -183,26 +231,76 @@ envoy_cc_library(
)

envoy_cc_library(
name = "transport_lib",
name = "auto_transport_lib",
srcs = [
"auto_transport_impl.cc",
],
hdrs = [
"auto_transport_impl.h",
],
deps = [
":buffer_helper_lib",
":framed_transport_lib",
":header_transport_lib",
":protocol_lib",
":transport_interface",
":unframed_transport_lib",
"//source/common/common:assert_lib",
],
)

envoy_cc_library(
name = "framed_transport_lib",
srcs = [
"framed_transport_impl.cc",
"header_transport_impl.cc",
"transport_impl.cc",
"unframed_transport_impl.cc",
],
hdrs = [
"framed_transport_impl.h",
],
deps = [
":buffer_helper_lib",
":transport_interface",
"//source/common/common:assert_lib",
],
)

envoy_cc_library(
name = "header_transport_lib",
srcs = [
"header_transport_impl.cc",
],
hdrs = [
"header_transport_impl.h",
"transport_impl.h",
"unframed_transport_impl.h",
],
deps = [
":app_exception_lib",
":buffer_helper_lib",
":metadata_lib",
":protocol_lib",
":transport_interface",
"//source/common/common:assert_lib",
"//source/common/singleton:const_singleton",
],
)

envoy_cc_library(
name = "unframed_transport_lib",
srcs = [
"unframed_transport_impl.cc",
],
hdrs = [
"unframed_transport_impl.h",
],
deps = [
":buffer_helper_lib",
":transport_interface",
"//source/common/common:assert_lib",
],
)

envoy_cc_library(
name = "transport_lib",
deps = [
":auto_transport_lib",
":framed_transport_lib",
":header_transport_lib",
":unframed_transport_lib",
],
)
2 changes: 1 addition & 1 deletion ...ers/network/thrift_proxy/protocol_impl.cc → ...etwork/thrift_proxy/auto_protocol_impl.cc
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
#include "extensions/filters/network/thrift_proxy/auto_protocol_impl.h"

#include <algorithm>

Expand Down
0 ...ters/network/thrift_proxy/protocol_impl.h → ...network/thrift_proxy/auto_protocol_impl.h
View file
File renamed without changes.
2 changes: 1 addition & 1 deletion ...rs/network/thrift_proxy/transport_impl.cc → ...twork/thrift_proxy/auto_transport_impl.cc
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "extensions/filters/network/thrift_proxy/transport_impl.h"
#include "extensions/filters/network/thrift_proxy/auto_transport_impl.h"

#include "envoy/common/exception.h"

Expand Down
2 changes: 0 additions & 2 deletions ...ers/network/thrift_proxy/transport_impl.h → ...etwork/thrift_proxy/auto_transport_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

#include "extensions/filters/network/thrift_proxy/transport.h"

#include "absl/types/optional.h"

namespace Envoy {
namespace Extensions {
namespace NetworkFilters {
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/thrift_proxy/binary_protocol_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include "envoy/buffer/buffer.h"
#include "envoy/common/pure.h"

#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
#include "extensions/filters/network/thrift_proxy/protocol.h"

namespace Envoy {
namespace Extensions {
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/thrift_proxy/compact_protocol_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#include "envoy/buffer/buffer.h"
#include "envoy/common/pure.h"

#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
#include "extensions/filters/network/thrift_proxy/protocol.h"

#include "absl/types/optional.h"

Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/network/thrift_proxy/config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@

#include "common/config/utility.h"

#include "extensions/filters/network/thrift_proxy/auto_protocol_impl.h"
#include "extensions/filters/network/thrift_proxy/auto_transport_impl.h"
#include "extensions/filters/network/thrift_proxy/binary_protocol_impl.h"
#include "extensions/filters/network/thrift_proxy/compact_protocol_impl.h"
#include "extensions/filters/network/thrift_proxy/decoder.h"
#include "extensions/filters/network/thrift_proxy/filters/filter_config.h"
#include "extensions/filters/network/thrift_proxy/filters/well_known_names.h"
#include "extensions/filters/network/thrift_proxy/framed_transport_impl.h"
#include "extensions/filters/network/thrift_proxy/protocol_impl.h"
#include "extensions/filters/network/thrift_proxy/stats.h"
#include "extensions/filters/network/thrift_proxy/transport_impl.h"
#include "extensions/filters/network/thrift_proxy/unframed_transport_impl.h"

namespace Envoy {
Expand Down
30 changes: 18 additions & 12 deletions source/extensions/filters/network/thrift_proxy/conn_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ void ConnectionManager::dispatch() {
try {
bool underflow = false;
while (!underflow) {
ThriftFilters::FilterStatus status = decoder_->onData(request_buffer_, underflow);
if (status == ThriftFilters::FilterStatus::StopIteration) {
FilterStatus status = decoder_->onData(request_buffer_, underflow);
if (status == FilterStatus::StopIteration) {
stopped_ = true;
break;
}
Expand Down Expand Up @@ -122,7 +122,7 @@ void ConnectionManager::onEvent(Network::ConnectionEvent event) {
}
}

ThriftFilters::DecoderFilter& ConnectionManager::newDecoderFilter() {
DecoderEventHandler& ConnectionManager::newDecoderEventHandler() {
ENVOY_LOG(trace, "new decoder filter");

ActiveRpcPtr new_rpc(new ActiveRpc(*this));
Expand All @@ -141,17 +141,16 @@ bool ConnectionManager::ResponseDecoder::onData(Buffer::Instance& data) {
return complete_;
}

ThriftFilters::FilterStatus
ConnectionManager::ResponseDecoder::messageBegin(MessageMetadataSharedPtr metadata) {
FilterStatus ConnectionManager::ResponseDecoder::messageBegin(MessageMetadataSharedPtr metadata) {
metadata_ = metadata;
first_reply_field_ =
(metadata->hasMessageType() && metadata->messageType() == MessageType::Reply);
return ProtocolConverter::messageBegin(metadata);
}

ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::fieldBegin(absl::string_view name,
FieldType field_type,
int16_t field_id) {
FilterStatus ConnectionManager::ResponseDecoder::fieldBegin(absl::string_view name,
FieldType field_type,
int16_t field_id) {
if (first_reply_field_) {
// Reply messages contain a struct where field 0 is the call result and fields 1+ are
// exceptions, if defined. At most one field may be set. Therefore, the very first field we
Expand All @@ -163,7 +162,7 @@ ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::fieldBegin(absl:
return ProtocolConverter::fieldBegin(name, field_type, field_id);
}

ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::transportEnd() {
FilterStatus ConnectionManager::ResponseDecoder::transportEnd() {
ASSERT(metadata_ != nullptr);

ConnectionManager& cm = parent_.parent_;
Expand Down Expand Up @@ -205,11 +204,12 @@ ThriftFilters::FilterStatus ConnectionManager::ResponseDecoder::transportEnd() {
break;
}

return ThriftFilters::FilterStatus::Continue;
return FilterStatus::Continue;
}

ThriftFilters::FilterStatus ConnectionManager::ActiveRpc::transportEnd() {
ASSERT(metadata_ != nullptr && metadata_->hasMessageType());
FilterStatus ConnectionManager::ActiveRpc::transportEnd() {
ASSERT(metadata_ != nullptr);
ASSERT(metadata_->hasMessageType());

parent_.stats_.request_.inc();

Expand All @@ -233,6 +233,12 @@ ThriftFilters::FilterStatus ConnectionManager::ActiveRpc::transportEnd() {
return decoder_filter_->transportEnd();
}

FilterStatus ConnectionManager::ActiveRpc::messageBegin(MessageMetadataSharedPtr metadata) {
metadata_ = metadata;

return event_handler_->messageBegin(metadata);
}

void ConnectionManager::ActiveRpc::createFilterChain() {
parent_.config_.filterFactory().createFilterChain(*this);
}
Expand Down
Loading

0 comments on commit 68d20b4

Please sign in to comment.