dubbo_proxy: Redefine DecoderFilter interface and add EncoderFilter support by gengleilei · Pull Request #7118 · envoyproxy/envoy

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

ActiveMessageDecoderFilter and ActiveMessagerEncodeFilter will use dual-filter.

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

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

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

fix comment

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

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

fix comment

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

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

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

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

Fix comments

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

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

total_size += size;

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

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

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

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

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

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

remove static_cast, don't use raw point here

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

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

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

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

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

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

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

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

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

remove static_cast ?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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>

Conversation

gengleilei commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented May 31, 2019

Uh oh!

lizan commented May 31, 2019

Uh oh!

gengleilei commented Jun 3, 2019

Uh oh!

zyfjeff commented Jun 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jun 13, 2019

Uh oh!

gengleilei commented Jun 18, 2019

Uh oh!

repokitteh-read-only bot commented Jun 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyfjeff commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengleilei commented May 31, 2019 •

edited

Loading

zyfjeff Jul 1, 2019 •

edited

Loading