dubbo_proxy: Refactor the DubboProxy filter by gengleilei · Pull Request #6410 · envoyproxy/envoy

gengleilei · 2019-03-28T07:22:46Z

Description: Refactor the DubboProxy filter
Risk Level: low
Testing: unit test
Docs Changes: inline
Release Notes: add routing capabilities for the Dubbo protocol
[Optional Fixes #Issue] #3998
[Optional Deprecated:]

gengleilei · 2019-03-28T11:34:19Z

/retest

repokitteh-read-only · 2019-03-28T11:34:26Z

🙀 failed invoking rebuild of ci/circleci: compile_time_options: 500 Internal Server Error
🙀 failed invoking rebuild of ci/circleci: clang_tidy: 500 Internal Server Error
🙀 failed invoking rebuild of ci/circleci: coverage: 500 Internal Server Error

🐱

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

see: more, trace.

zyfjeff · 2019-03-29T01:06:15Z

Code coverage 97.4 is lower than limit of 97.5

https://circleci.com/gh/envoyproxy/envoy/190601?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Improve the code coverage, please

zyfjeff · 2019-03-29T02:48:20Z

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

What's the relationship between response_success_ and response_exception_ stats?

When message_type equal MessageType::Exception, is it possible that response_status equal OK ?

Response_status == OK only means that the Dubbo framework handled the request correctly. The RpcResponseType represents the result of the business layer's processing of the request. ResponseWithException means that the business layer threw an exception when processing the request, response_exception_ is used to count exceptions thrown by the business layer.

Yes，it possible.

zyfjeff · 2019-03-29T02:52:40Z

Add related doc and release note, please

zyfjeff · 2019-04-01T03:24:33Z

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

Why not check status in other filter callbacks?
What if I return StopIteration in other filter callbacks?

this is because StopIteration is most likely to occur in the messageEnd callback, where the user is already presented with enough information to satisfy routing requirements.

if StopIteration is returned in other filter callback, the decoder will stop decoding unless the continueDecoding interface is called.

I think this check should be done in the last filter callback, not here.

The applyDecoderFilters checking has been moved to the last filter callback.

zyfjeff · 2019-04-01T03:28:05Z

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

Why not call this method in the last filter callback?
What should I do if I reject the current request in the last filter callback?

since the Decoder has decoded the request data at this time, the statistical item in finalizeRequest function is specific to the request.

if StopIteration is returned in other filter callback, the decoder will stop decoding unless the continueDecoding interface is called.

if a local reply is made in last filter callback, is this request still valid? If it is not valid, the finalizeRequest will perform stats statistics.

If the last filter has a local reply, who is responsible for the deferred ActiveMessage?

The finalizeRequest call has been moved to the last filter callback.

zyfjeff · 2019-04-01T03:29:53Z

source/extensions/filters/network/dubbo_proxy/BUILD

Where to use?

It's useless. I deleted it.

zyfjeff · 2019-04-01T03:40:24Z

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

route is shared_ptr, you can copy it directly.

zyfjeff · 2019-04-02T12:38:51Z

api/envoy/config/filter/network/dubbo_proxy/v2alpha1/route.proto

zyfjeff · 2019-04-02T12:39:23Z

api/envoy/config/filter/network/dubbo_proxy/v2alpha1/route.proto

zyfjeff · 2019-04-02T12:39:36Z

api/envoy/config/filter/network/dubbo_proxy/v2alpha1/route.proto

zyfjeff · 2019-04-02T12:40:54Z

docs/root/intro/version_history.rst

Alphabetical order

gengleilei · 2019-04-04T09:15:02Z

/retest

repokitteh-read-only · 2019-04-04T09:15:09Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

gengleilei · 2019-04-08T02:37:28Z

@zyfjeff Is there any other code review opinion ?

zyfjeff · 2019-04-08T02:20:06Z

docs/root/intro/version_history.rst

 * config: use Envoy cpuset size to set the default number or worker threads if :option:`--cpuset-threads` is enabled.
 * config: added support for :ref:`initial_fetch_timeout <envoy_api_field_core.ConfigSource.initial_fetch_timeout>`. The timeout is disabled by default.
 * cors: added :ref:`filter_enabled & shadow_enabled RuntimeFractionalPercent flags <cors-runtime>` to filter.
+* dubbo_proxy: add routing capabilities for the Dubbo protocol.


dubbo_proxy: Support dubbo proxy filter?

This is the first appearance of dubbo proxy.

good, i have modified it.

zyfjeff · 2019-04-08T03:16:15Z

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

+
+    DecoderEventHandler& handler_;
+  };
+  typedef std::unique_ptr<ActiveRequest> ActiveRequestPtr;


zyfjeff · 2019-04-08T03:31:24Z

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

+  Protocol& protocol_;
+  DecoderStateMachinePtr state_machine_;
+  bool decode_started_ = false;
+  DecoderCallbacks* decoder_callbacks_;


If decoder_callbacks_ is not nullptr, it should be referenced here.

zyfjeff · 2019-04-08T03:59:09Z

LGTM, just some nits
@lizan take a look when you have time

lizan

Apologize for delay, assuming @zyfjeff takes thorough review on the logic itself, some C++/style comments only.

lizan · 2019-04-10T09:11:12Z

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

lizan · 2019-04-10T09:19:59Z

docs/root/intro/version_history.rst

can you merge master and bump this to 1.11?

lizan · 2019-04-10T09:20:31Z

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

absl::flat_hash_map? (ditto below)

lizan · 2019-04-10T09:21:18Z

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

I've deleted it.

lizan · 2019-04-10T09:22:10Z

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

This file has slightly low coverage:
https://195194-65214191-gh.circle-artifacts.com/0/coverage/coverage.source_extensions_filters_network_dubbo_proxy_config.cc.html

can you add test for failure case? If they are validated via PGV, then just ASSERT is enough.

yes, ASSERT is enough, i have modified it.

lizan · 2019-04-10T09:23:35Z

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

nit: constexpr

lizan · 2019-04-10T09:27:03Z

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

The style here seems inconsistent (PascalCase vs camelCase), what is the motivation for that?

sorry, it should use PascalCase, i have modified it.

lizan · 2019-04-10T09:27:32Z

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

nit: {} initialize

lizan · 2019-04-11T08:05:18Z

docs/root/intro/version_history.rst

ref link to the document?

yes, thank you for reminding me.

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

gengleilei · 2019-04-12T04:41:44Z

@lizan Is there any other code review opinion ?

gengleilei requested a review from lizan as a code owner March 28, 2019 07:22

lizan self-assigned this Mar 28, 2019

lizan requested a review from zyfjeff March 28, 2019 23:44

zyfjeff reviewed Mar 29, 2019

View reviewed changes

zyfjeff reviewed Apr 1, 2019

View reviewed changes

zyfjeff suggested changes Apr 2, 2019

View reviewed changes

gengleilei force-pushed the feature-refactor-filter-ex branch from 1a2f832 to 21a3b43 Compare April 4, 2019 06:21

zyfjeff reviewed Apr 8, 2019

View reviewed changes

lizan suggested changes Apr 10, 2019

View reviewed changes

gengleilei force-pushed the feature-refactor-filter-ex branch from f826741 to 2ac8278 Compare April 11, 2019 05:53

lizan reviewed Apr 11, 2019

View reviewed changes

gengleilei added 9 commits April 12, 2019 09:41

Refactor the DubboProxy filter

082fd85

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

Fix spelling errors

4168b62

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

Rename DecoderTest and DecoderStateMachineTest

e79ffbe

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

Add some unit test cases.

5d6ab59

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

Optimize the code

058cc87

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

Add doc and release notes.

dfa7a22

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

Change dubbo_proxy library type to api_proto_library_internal

db21056

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

Fix doc compilation errors

a710bd9

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

Fix doc compilation errors

d487325

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

gengleilei added 7 commits April 12, 2019 09:41

Fix format error

f28f98f

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

Fix doc compilation errors

3a1e245

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

Resolve version_history.rst conflict

21d169c

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

Optimize the code

5775c7d

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

Optimize the code

efc8acb

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

Add a link for the Dubbo proxy filter

35bc84f

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

Resolve version_history.rst conflict

a5633b8

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

gengleilei force-pushed the feature-refactor-filter-ex branch from 21f5439 to a5633b8 Compare April 12, 2019 01:47

lizan approved these changes Apr 12, 2019

View reviewed changes

lizan merged commit db7f124 into envoyproxy:master Apr 12, 2019

Conversation

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

Uh oh!

Uh oh!

gengleilei commented Mar 28, 2019

Uh oh!

repokitteh-read-only bot commented Mar 28, 2019

Uh oh!

zyfjeff commented Mar 29, 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!

zyfjeff commented Mar 29, 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!

zyfjeff Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Apr 4, 2019

Uh oh!

repokitteh-read-only bot commented Apr 4, 2019

Uh oh!

gengleilei commented Apr 8, 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!

gengleilei commented Mar 28, 2019 •

edited

Loading

zyfjeff Apr 1, 2019 •

edited

Loading