dubbo_proxy: Redefine DecoderFilter interface and add EncoderFilter support #7118

gengleilei · 2019-05-31T11:48:08Z

Description: Redefine DecoderFilter interface and add EncoderFilter support
Risk Level: low
Testing: unit test
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

Redefining DecoderFilter is mainly for 2 reasons:

The original interface is not clear and complicated for the user, for example, the user needs to understand the meaning of transportBegin, transportEnd, messageBegin, messageEnd, transferHeaderTo, transferBodyTo, and we want to minimize this cost, so we decided to provide only one after internal discussion: onMessageDecoded interface.
We used to have an internal implementation version and submitted a version to the community, but this obviously wastes a lot of development resources, so we decided to merge the two versions, mainly based on the open source version.

As mentioned above, we merged the internal and open source versions. EncoderFilter is currently needed internally, such as the retry mechanism. we will consider adding more features to Dubbo later, similar to HTTP, so EncoderFilter is also necessary. we want to bring more features to users around Dubbo.

jmarantz · 2019-05-31T12:46:09Z

@lizan the author requested a review from you, but do you prefer to have a non-senior maintainer read it first?

@gengleilei drive-by comment: this meaty PR probably deserves more content in the description. Why is the DecoderFilter being redefined, and why is there now a need for an EncoderFilter?

lizan · 2019-05-31T13:36:18Z

/assign @zyfjeff
Can you take a first pass?

gengleilei · 2019-06-03T03:17:32Z

@jmarantz sorry, I've added some more detailed instructions on redefining DecoderFilter and supporting EncoderFilter.

zyfjeff · 2019-06-06T06:56:47Z

@lizan I'm sorry, recently a little busy, I will be review recently

zyfjeff · 2019-06-06T07:01:41Z

source/extensions/filters/network/dubbo_proxy/active_message.h

+
+protected:
+  ActiveMessage& parent_;
+  const bool dual_filter_ : 1;


ActiveMessageDecoderFilter and ActiveMessagerEncodeFilter will use dual-filter.

zyfjeff · 2019-06-06T07:07:06Z

source/extensions/filters/network/dubbo_proxy/conn_manager.cc

@@ -32,21 +31,6 @@ Network::FilterStatus ConnectionManager::onData(Buffer::Instance& data, bool end

  if (end_stream) {
    ENVOY_CONN_LOG(trace, "downstream half-closed", read_callbacks_->connection());
-
-    // Downstream has closed. Unless we're waiting for an upstream connection to complete a oneway
-    // request, close. The special case for oneway requests allows them to complete before the


Why remove

Downstream half closed, does it need to wait for the response of the upstream?

sorry, the code was deleted by mistake while merging, I have restored it.

zyfjeff · 2019-06-06T07:15:11Z

source/extensions/filters/network/dubbo_proxy/serializer.h

+   * If successful, the RpcInvocation removed from the buffer
+   *
+   * @param buffer the currently buffered dubbo data
+   * @body_size the complete RpcInvocation size


fix comment

zyfjeff · 2019-06-06T07:15:23Z

source/extensions/filters/network/dubbo_proxy/serializer.h

+   * If successful, the RpcResult removed from the buffer
+   *
+   * @param buffer the currently buffered dubbo data
+   * @body_size the complete RpcResult size


fix comment

zyfjeff · 2019-06-06T07:19:41Z

source/extensions/filters/network/dubbo_proxy/filters/filter.h

+/**
+ * Encoder filter interface.
+ */
+class EncoderFilter : public StreamEncoder, public FilterBase {


Please add EncoderFilter unit tests

stale · 2019-06-13T07:38:25Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

gengleilei · 2019-06-18T12:47:42Z

/retest

repokitteh-read-only · 2019-06-18T12:47:46Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #7118 (comment) was created by @gengleilei.

see: more, trace.

zyfjeff · 2019-06-20T02:07:41Z

source/extensions/filters/network/dubbo_proxy/conn_manager.cc

-        stopped_ = true;
-        break;
-      }
+      decoder_->onData(request_buffer_, underflow);


Why doesn't care about the return value for onData here?

At present, when Decoder processes data, it either finishes processing or needs to wait until the data is entered, so there is no stop.

zyfjeff · 2019-06-20T02:12:25Z

source/extensions/filters/network/dubbo_proxy/decoder_event_handler.h

+  // continueDecoding()/continueEncoding() MUST be called if continued filter iteration is desired.
+  StopIteration,
+  // Continue iteration to remaining filters, but ignore any subsequent data or trailers. This
+  // results in creating a header only request/response.


Fix comments

zyfjeff · 2019-06-20T02:13:42Z

source/extensions/filters/network/dubbo_proxy/dubbo_hessian2_serializer_impl.cc

+  size_t total_size = 0, size;
+  // TODO(zyfjeff): Add format checker
+  std::string dubbo_version = HessianUtils::peekString(buffer, &size);
+  total_size = total_size + size;


total_size += size;

zyfjeff · 2019-06-20T02:17:17Z

source/extensions/filters/network/dubbo_proxy/dubbo_hessian2_serializer_impl.cc

+  std::string method_name = HessianUtils::peekString(buffer, &size, total_size);
+  total_size = total_size + size;
+
+  ContextImpl* context_impl = static_cast<ContextImpl*>(context.get());


ASSERT(context)? Why do you want to take raw pointer here?

sorry，there is no need to convert, this part is left out when modify the code.

zyfjeff · 2019-06-20T02:17:47Z

/wait

zyfjeff · 2019-06-24T02:25:05Z

source/extensions/filters/network/dubbo_proxy/app_exception.h

-  const ResponseStatus status_;
+  ResponseType encode(MessageMetadata& metadata, DubboProxy::Protocol& protocol,
+                      Buffer::Instance& buffer) const override {
+    ASSERT(buffer.length() == 0);


I can't append data to a buffer?

yes, at present, I haven't thought of any scenarios that require append data, and I can let go of this restriction if necessary.

zyfjeff · 2019-06-24T02:30:36Z

source/extensions/filters/network/dubbo_proxy/conn_manager.cc

@@ -145,7 +139,7 @@ void ConnectionManager::sendLocalReply(MessageMetadata& metadata,

  Buffer::OwnedImpl buffer;
  const DubboFilters::DirectResponse::ResponseType result =
-      response.encode(metadata, *protocol_, *deserializer_, buffer);
+      response.encode(metadata, *protocol_, buffer);


zyfjeff · 2019-06-24T02:36:02Z

source/extensions/filters/network/dubbo_proxy/dubbo_protocol_impl.cc

+                                   MessageMetadataSharedPtr metadata) {
+  ASSERT(serializer_);
+
+  ContextImpl* context_impl = static_cast<ContextImpl*>(context.get());


remove static_cast, don't use raw point here

zyfjeff · 2019-06-24T02:39:05Z

source/extensions/filters/network/dubbo_proxy/message_impl.h

+namespace NetworkFilters {
+namespace DubboProxy {
+
+struct ContextBase : public Context {


Why not use class? this class is not a simple POD type, also contains some virtual method

Because it's simple, you don't need to actively add public declarations.

I think here should use the class, this is not a simple POD type. Can't go to easy access to the data members of the break of object-oriented rules

zyfjeff · 2019-06-24T02:40:06Z

source/extensions/filters/network/dubbo_proxy/message_impl.h

+};
+
+struct ContextImpl : public ContextBase {
+  ContextImpl() : ContextBase() {}


zyfjeff · 2019-06-24T02:44:47Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

@@ -137,19 +141,22 @@ MethodRouteEntryImpl::~MethodRouteEntryImpl() {}

 RouteConstSharedPtr MethodRouteEntryImpl::matches(const MessageMetadata& metadata,
                                                  uint64_t random_value) const {
-  if (metadata.hasHeaders() && !RouteEntryImplBase::headersMatch(metadata.headers())) {
+  ASSERT(metadata.hasInvocationInfo());
+  const auto invocation = static_cast<const RpcInvocationImpl*>(&metadata.invocation_info());


Why use a raw point?

zyfjeff · 2019-06-27T01:36:19Z

Please merge the master to resolve conflicts

zyfjeff · 2019-06-27T01:54:57Z

source/extensions/filters/network/dubbo_proxy/config.cc

+  Router::RouteMatcherPtr matcher =
+      Router::NamedRouteMatcherConfigFactory::getFactory(type).createRouteMatcher(
+          config.route_config(), context);
+  route_matcher_ = std::move(matcher);


route_matcher_ = Router::NamedRouteMatcherConfigFactory::getFactory(type).createRouteMatcher( config.route_config(), context);

zyfjeff · 2019-06-27T02:17:35Z

source/extensions/filters/network/dubbo_proxy/router/router_impl.h

-                                     SerializationType serialization_type) override;
-  Network::FilterStatus messageEnd(MessageMetadataSharedPtr metadata) override;
+
+  FilterStatus onMessageDecoded(MessageMetadataSharedPtr metadata, ContextSharedPtr ctx) override;


const ContextSharedPtr?

const cannot be used here because it changes the value of message_origin_buffer_ in the Context, as used in the onMessageDecoded function:
upstream_request_buffer_.move(ctx->message_origin_data(), ctx->message_size());

zyfjeff · 2019-06-27T02:20:46Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

@@ -137,19 +141,22 @@ MethodRouteEntryImpl::~MethodRouteEntryImpl() {}

 RouteConstSharedPtr MethodRouteEntryImpl::matches(const MessageMetadata& metadata,
                                                  uint64_t random_value) const {
-  if (metadata.hasHeaders() && !RouteEntryImplBase::headersMatch(metadata.headers())) {
+  ASSERT(metadata.hasInvocationInfo());
+  const auto& invocation = static_cast<const RpcInvocationImpl&>(metadata.invocation_info());


remove static_cast ?

It cannot be deleted here because the static_cast is needed to complete the conversion of the base class reference to the derived class reference, which will fail to compile after deletion.

Downcasting, Should use dynamic_cast and judge whether the result is nullptr after the cast, rather than use staic_cast directly

zyfjeff · 2019-06-27T02:21:24Z

source/extensions/filters/network/dubbo_proxy/router/route_matcher.cc

@@ -81,13 +83,15 @@ bool ParameterRouteEntryImpl::matchParameter(absl::string_view request_data,

 RouteConstSharedPtr ParameterRouteEntryImpl::matches(const MessageMetadata& metadata,
                                                     uint64_t random_value) const {
-  if (!metadata.hasParameters()) {
+  ASSERT(metadata.hasInvocationInfo());
+  const auto& invocation = static_cast<const RpcInvocationImpl&>(metadata.invocation_info());


remove static_cast ?

zyfjeff · 2019-06-27T02:22:54Z

source/extensions/filters/network/dubbo_proxy/router/route.h

+
+  const std::string& fromType(RouteMatcherType type) const {
+    const auto& itor = routeMatcherNameMap.find(type);
+    if (itor != routeMatcherNameMap.end()) {


ASSERT(itor != routeMatcherNameMap.end()) return itor->second;

zyfjeff · 2019-06-27T02:23:59Z

source/extensions/filters/network/dubbo_proxy/protocol_constants.h

+  const std::string& fromType(ProtocolType protocol_type, SerializationType type) const {
+    const auto& itor = protocolSerializerTypeNameMap.find(generateKey(protocol_type, type));
+    if (itor != protocolSerializerTypeNameMap.end()) {
+      return itor->second;


ASSERT(itor != protocolSerializerTypeNameMap.end()) return itor->second;

zyfjeff · 2019-06-27T02:24:35Z

source/extensions/filters/network/dubbo_proxy/protocol_constants.h

+  const std::string& fromType(SerializationType type) const {
+    const auto& itor = serializerTypeNameMap.find(type);
+    if (itor != serializerTypeNameMap.end()) {
+      return itor->second;


ASSERT(itor != serializerTypeNameMap.end()) return itor->second;

zyfjeff · 2019-06-27T02:25:14Z

source/extensions/filters/network/dubbo_proxy/protocol_constants.h

+  const std::string& fromType(ProtocolType type) const {
+    const auto& itor = protocolTypeNameMap.find(type);
+    if (itor != protocolTypeNameMap.end()) {
+      return itor->second;


ASSERT(itor != protocolTypeNameMap.end()) return itor->second;

zyfjeff · 2019-06-27T02:26:29Z

source/extensions/filters/network/dubbo_proxy/metadata.h

-  absl::optional<std::string> group_;
-  ParameterValueMapPtr parameter_map_;
-  HeaderMapPtr headers_; // attachment
+  uint8_t serialization_type_{static_cast<uint8_t>(SerializationType::Hessian2)};


why not use SerializationType

To be compatible with the internal HSF protocol, both HSF and Dubbo define SerializationType, but with different values.

zyfjeff · 2019-06-27T02:29:13Z

source/extensions/filters/network/dubbo_proxy/message_impl.h

+
+protected:
+  std::string service_name_;
+  absl::optional<std::string> method_name_;


remove optional? The method name why will not exist?

zyfjeff · 2019-06-27T02:30:05Z

/ wait

zyfjeff · 2019-07-01T01:25:27Z

/ wait

zyfjeff

LGTM, @lizan Passed the first review

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

gengleilei · 2019-07-08T03:03:46Z

@lizan Please help to review, thanks.

lizan · 2019-07-09T23:43:59Z

source/extensions/filters/network/dubbo_proxy/active_message.cc

+  ASSERT(encoder_filter_action_ != nullptr);
+
+  if (!local_response_sent_) {
+    std::list<ActiveMessageEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, state);


put this in the for statement below?

lizan · 2019-07-09T23:45:38Z

source/extensions/filters/network/dubbo_proxy/app_exception.h

+                      Buffer::Instance& buffer) const override {
+    ASSERT(buffer.length() == 0);
+
+    ENVOY_LOG(debug, "err {}", what());


make the log message more descriptive?

lizan · 2019-07-09T23:48:42Z

source/extensions/filters/network/dubbo_proxy/serializer_impl.h

+  ~RpcResultImpl() override = default;
+
+  bool hasException() const override { return has_exception_; }
+  void SetException(bool has_exception) { has_exception_ = has_exception; }


setException

lizan · 2019-07-09T23:50:09Z

source/extensions/filters/network/dubbo_proxy/message.h

-  virtual ResponseStatus responseStatus() const PURE;
+  virtual ~RpcResult() = default;
+  virtual bool hasException() const PURE;
+  virtual bool hasValue() const PURE;


where is this used?

sorry，hasValue function is not used, I have deleted it.

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Fix merge race of #7118 and #7447 Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Lizan Zhou <lizan@tetrate.io>

gengleilei requested a review from lizan as a code owner May 31, 2019 11:48

jmarantz assigned lizan May 31, 2019

repokitteh-read-only bot assigned zyfjeff May 31, 2019

lizan requested a review from zyfjeff May 31, 2019 13:36

gengleilei force-pushed the feature-redefine-decoder-and-support-encoder branch from 6a505aa to 128e382 Compare June 3, 2019 02:36

zyfjeff suggested changes Jun 6, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 13, 2019

gengleilei force-pushed the feature-redefine-decoder-and-support-encoder branch from 128e382 to 223c493 Compare June 17, 2019 16:43

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 17, 2019

zyfjeff suggested changes Jun 20, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jun 20, 2019

zyfjeff suggested changes Jun 24, 2019

View reviewed changes

gengleilei force-pushed the feature-redefine-decoder-and-support-encoder branch from 7c266f8 to e84dda6 Compare June 26, 2019 13:34

zyfjeff removed their assignment Jun 27, 2019

zyfjeff suggested changes Jun 27, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 27, 2019

gengleilei force-pushed the feature-redefine-decoder-and-support-encoder branch from e84dda6 to 550ae32 Compare June 27, 2019 05:54

repokitteh-read-only bot removed the waiting label Jun 27, 2019

repokitteh-read-only bot added waiting and removed waiting labels Jul 1, 2019

zyfjeff previously approved these changes Jul 1, 2019

View reviewed changes

gengleilei added 6 commits July 8, 2019 10:44

Redefine DecoderFilter interface and add EncoderFilter support

35ffbee

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Add unit test cases for EncoderFilter

cc3ae93

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Optimize the code

1bc7aef

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Optimize the code

2e98a49

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Optimize the code

fb65823

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

Optimize the code

a5ce22a

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

gengleilei dismissed zyfjeff’s stale review via a5ce22a July 8, 2019 03:00

gengleilei force-pushed the feature-redefine-decoder-and-support-encoder branch from 88a08f1 to a5ce22a Compare July 8, 2019 03:00

lizan reviewed Jul 9, 2019

View reviewed changes

lizan added the waiting label Jul 9, 2019

Optimize the code

c7568cc

Signed-off-by: leilei.gll <leilei.gll@alibaba-inc.com>

repokitteh-read-only bot removed the waiting label Jul 10, 2019

lizan approved these changes Jul 15, 2019

View reviewed changes

lizan merged commit e442ba5 into envoyproxy:master Jul 15, 2019

lizan mentioned this pull request Jul 15, 2019

dubbo: tidy fix #7587

Merged

zuercher pushed a commit that referenced this pull request Jul 16, 2019

dubbo: clang-tidy fix (#7587)

49635f4

Fix merge race of #7118 and #7447 Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Lizan Zhou <lizan@tetrate.io>

dubbo_proxy: Redefine DecoderFilter interface and add EncoderFilter support #7118

dubbo_proxy: Redefine DecoderFilter interface and add EncoderFilter support #7118

Conversation

gengleilei commented May 31, 2019 • edited Loading

jmarantz commented May 31, 2019

lizan commented May 31, 2019

gengleilei commented Jun 3, 2019

zyfjeff commented Jun 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 13, 2019

gengleilei commented Jun 18, 2019

repokitteh-read-only bot commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyfjeff commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyfjeff commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyfjeff Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyfjeff commented Jun 27, 2019

zyfjeff commented Jul 1, 2019

zyfjeff left a comment

Choose a reason for hiding this comment

gengleilei commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gengleilei commented May 31, 2019 •

edited

Loading

zyfjeff Jul 1, 2019 •

edited

Loading