rocketmq_proxy: implement rocketmq proxy #9503

aaron-ai · 2019-12-27T07:01:54Z

Implement rocketmq proxy

Description: implement rocketmq proxy
Risk Level: Low
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] #9431

mattklein123 · 2019-12-29T18:10:24Z

@aaron-ai thanks for your interest in contributing this awesome feature! I'm going to be honest in saying that we cannot accept an 8K line PR with no prior notice and no plan for reviewing. Do you have any maintainers in mind who would be willing to sponsor reviewing this PR and seeing that it get merged? Thank you.

stale · 2020-01-06T03:14:44Z

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!

aaron-ai · 2020-01-06T09:25:01Z

@mattklein123 Thanks for your feedback first! This pull request does look too big at first sight, but more than half code of lines are test cases to meet 100% test coverages goal required here. The main source parts are self-contained and generally feature-implement-purpose. Once understanding core concepts of Apache RocketMQ, reviewing this PR will not be as challenging as initial thought.

zyfjeff · 2020-01-06T09:27:34Z

CODEOWNERS

@@ -16,6 +16,8 @@
 extensions/filters/common/original_src @snowp @klarose
 # dubbo_proxy extension
 /*/extensions/filters/network/dubbo_proxy @zyfjeff @lizan
+# rocketmq_proxy extension
+/*/extensions/filters/network/rocketmq_proxy @aaron-ai @lizhanhui


@aaron-ai There must be a maintainer in code owner https://github.com/envoyproxy/envoy/blob/master/EXTENSION_POLICY.md

Thanks @zyfjeff, we are still looking for a maintainer who has the convenience of sponsoring.

@aaron-ai @lizhanhui I don't have the bandwidth right now to do a full review for this and I doubt any of the @envoyproxy/maintainers do either, but they can speak for themselves. However, if @zyfjeff is willing to take point and do a thorough review I'm happy to do a skim afterwords and sponsor. @zyfjeff does that sound OK to you?

@mattklein123
I would be happy to review this, I used to write rocketmq c++ sdk, so I think I can do it

I would be happy to review this, I used to write rocketmq c++ sdk, so I think I can do it

Awesome. Thank you so much! :)

zyfjeff

I can help review, but unfortunately I am not a maintainer

zyfjeff · 2020-01-06T16:55:22Z

api/envoy/config/filter/network/rocketmq_proxy/v2alpha1/rocketmq_proxy.proto

+message RocketmqProxy {
+  string stat_prefix = 1 [(validate.rules).string = {min_bytes: 1}];
+
+  bool develop_mode = 2;


Please add some comments to explain this field

Thanks, I'll fix it.

zyfjeff · 2020-01-06T17:06:20Z

include/envoy/config/grpc_mux.h

+   * The only difference between addToWatch() and addOrUpdateWatch() is that the 'resources' here
+   * means the *extra* resources we interested in.
+   */
+  virtual Watch* addToWatch(const std::string& type_url, Watch* watch,


Is it possible to split a separate PR for review?

I do not believe that this code is part of the rocketmq_proxy filter, and this part of the code does not pass review in #8984 .

Is it possible to split a separate PR for review?

I do not believe that this code is part of the rocketmq_proxy filter, and this part of the code does not pass review in #8984 .

I would start a new PR about this part. Thank you.

Is it possible to split a separate PR for review?
I do not believe that this code is part of the rocketmq_proxy filter, and this part of the code does not pass review in #8984 .

I would start a new PR about this part. Thank you.

#9626

dio · 2020-01-07T02:10:57Z

source/extensions/filters/network/rocketmq_proxy/BUILD

+        "message_queue.h",
+        "topic_route.h",
+    ],
+    external_deps = ["rapidjson"],


Let's not depend on rapidjson. See: #4705

I also had the same ideas, 😄

zyfjeff · 2020-01-07T02:14:28Z

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

+#include "extensions/filters/network/rocketmq_proxy/conn_manager.h"
+#include "extensions/filters/network/rocketmq_proxy/topic_route.h"
+
+#include "rapidjson/stringbuffer.h"


Envoy is now actively trying to get rid of its depends on rapidjson #4705 we should convert to JSON using protobuf

Now replaced with Protobuf.

zyfjeff · 2020-01-07T02:15:51Z

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

+  connection_manager_.stats().request_active_.inc();
+  auto code = static_cast<RequestCode>(request_->code());
+  switch (code) {
+  case RequestCode::POP_MESSAGE: {


Redundant code that can be put into a common function for stats statistics.

zyfjeff · 2020-01-10T09:16:37Z

source/extensions/filters/network/rocketmq_proxy/well_known_names.h

+
+class RocketmqConstants {
+public:
+  static const std::string CLUSTER_TYPE;


Please replace it with the idiom used by the Envoy,

struct RocketmqValues { const std::string CLUSTER_TYPE{xxxx}; }; using RocketmqConstants = ConstSingleton<RocketmqValues>;

Nice catch.

It has been fixed.

zyfjeff · 2020-01-10T09:24:26Z

source/extensions/filters/network/rocketmq_proxy/topic_route.h

+
+  TopicRouteData() = default;
+
+  TopicRouteData(std::vector<QueueData>& queueData, std::vector<BrokerData>& brokerData)


please replace std::vector<QueueData>& queueData to std::vector<QueueData>&& queueData

It is not easy to read

std::vector<QueueData> test TopicRouteData tt(test); // The following code is not aware that test has been moved and will continue to use test

But it's much easier to read if you go like this

std::vector<QueueData> test TopicRouteData tt(std::move(test)); // I knew from looking at the code that test had been moved.

OK, let's use the move way.

zyfjeff · 2020-01-10T11:12:29Z

source/extensions/filters/network/rocketmq_proxy/topic_route.h

+public:
+  BrokerData(const std::string& cluster, const std::string& brokerName,
+             std::unordered_map<int64_t, std::string>& brokerAddrs)
+      : cluster_(cluster), broker_name_(brokerName), broker_addrs_(brokerAddrs) {}


broker_addrs_(brokerAddrs) Do we need a copy here?

All right. Will use move to avoid copy

zyfjeff · 2020-01-10T12:47:27Z

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

+  Upstream::ClusterInfoConstSharedPtr cluster_info_;
+  UpstreamRequestPtr upstream_request_;
+
+  absl::string_view target_host_;


How do you ensure that the string referenced by target_host_ is always valid?

target_host_ referenced can only be either empty or a valid target host. In case it is empty, host selection is completely subject to the LB. If it is a valid target host address, the request will be routed to it.

Refer to this comment

I mean whether the memory referenced by target_host_ is valid, not the content.

zyfjeff · 2020-01-10T13:23:39Z

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

+
+  std::string target_host = app_load_balancer_ptr_->lb(request);
+  if (!target_host.empty()) {
+    createActiveMessage(request).sendRequestToUpstream(target_host);


sendRequestToUpstream(target_host) -> createFilterChain(target_host); -> connection_manager_.config().createRouter(target_host);

RouterPtr createRouter(absl::string_view target_host) override { return std::make_unique<RouterImpl>(context_.clusterManager(), target_host); } class RouterImpl { private: absl::string_view target_host_; }

Finally, target_host_ in RouterImpl refers to the target_host here, but the target_host here is released when it leaves the scope, resulting an undefined behavior in routerimpl.target_host_ refers to invalid memory.

Ah, I get your point. This is a bug. target_host_ should be of std::string, not string_view.

string_view has been fixed already.

aaron-ai · 2020-03-06T09:19:23Z

@zyfjeff All issues above have been fixed.

zyfjeff · 2020-03-17T12:42:57Z

source/extensions/filters/network/rocketmq_proxy/router/router_impl.cc

+RouterImpl::UpstreamRequest::UpstreamRequest(RouterImpl& router,
+                                             Tcp::ConnectionPool::Instance& connectionPool)
+    : router_(router), connectionPool_(connectionPool) {
+  (void)connectionPool_;


remove, Didn't see this variable used

zyfjeff · 2020-03-17T12:50:07Z

source/extensions/filters/network/rocketmq_proxy/router/router_impl.cc

+  return false;
+}
+
+uint32_t RouterImpl::hostSelectionRetryCount() const {


Well, I think you should implement shouldSelectAnotherHost, not hostSelectionRetryCount.

zyfjeff · 2020-03-17T12:54:57Z

source/extensions/filters/network/rocketmq_proxy/router/router_impl.cc

+  active_message_->onReset();
+}
+
+bool RouterImpl::shouldSelectAnotherHost(const Upstream::Host& host) {


I think that subset should be used to select the endpoint

OK, I will use subset instead.

aaron-ai · 2020-03-24T04:15:34Z

@zyfjeff All issues above have been fixed.

zyfjeff

@aaron-ai good work, I have finished review, I have no issues.

aaron-ai · 2020-03-27T14:55:40Z

All code of rocketmq's proxy have been reviewed by @zyfjeff , are you available to sponsor our extension recently ? @lizan

lizan · 2020-04-03T03:18:54Z

Can you merge master once? I can sponsor meanwhile until someone from Alibaba becomes maintainer, but I don't have enough bandwidth to review this week.

aaron-ai · 2020-04-03T05:14:39Z

Can you merge master once? I can sponsor meanwhile until someone from Alibaba becomes maintainer, but I don't have enough bandwidth to review this week.

OK

htuch · 2020-04-07T14:53:08Z

Please merge master to pick up #10672. We no longer accept changes to v2 (without explicit exception), so any API modifications should happen in v3. If this PR is adding a new proto, please follow the updated instructions in https://github.com/envoyproxy/envoy/blob/master/api/STYLE.md#adding-an-extension-configuration-to-the-api.

lizan

Can you merge master again to pick 1.14.0 release and then add a release note for 1.15.0?

lizan · 2020-04-12T06:09:27Z

docs/root/configuration/listeners/network_filters/rocketmq_proxy_filter.rst

+The rocketmq proxy filter decodes the RPC protocol between rocketmq clients
+and nameserver/broker. 
+
+* :ref:`v2 API reference <envoy_api_msg_config.filter.network.rocketmq_proxy.v2alpha1.RocketMQProxy>`


I'll update the docs.

lizan · 2020-04-12T06:09:46Z

generated_api_shadow/BUILD

@@ -77,6 +77,7 @@ proto_library(
        "//envoy/config/filter/network/rate_limit/v2:pkg",
        "//envoy/config/filter/network/rbac/v2:pkg",
        "//envoy/config/filter/network/redis_proxy/v2:pkg",
+        "//envoy/config/filter/network/rocketmq_proxy/v2alpha1:pkg",


lizan · 2020-04-12T06:10:39Z

source/extensions/filters/network/rocketmq_proxy/codec.cc

+namespace NetworkFilters {
+namespace RocketmqProxy {
+
+const uint32_t RemotingCommand::SHIFT_RPC = 0;


static constexpr

lizan · 2020-04-12T06:13:01Z

source/extensions/filters/network/rocketmq_proxy/codec.cc

+  case RequestCode::PopMessage: {
+    ASSERT(fieldValuePair.contains("extFields"));
+    const auto& extFields = fieldValuePair.at("extFields");
+    std::unique_ptr<PopMessageRequestHeader> popMessageRequestHeader =


pop_message_request_header, please fix all local variable cases in this PR

lizhanhui · 2020-04-15T13:47:40Z

Hi, @lizan, Release note is added to current.rst and constexpr-issues are fixed.

One more TODO is also inserted, indicating future work that is planned.

aaron-ai · 2020-04-15T13:57:40Z

More docs have been added. A small part of code are not covered by UT, and I will implement them tomorrow.

lizhanhui · 2020-04-21T13:21:36Z

@lizan Unit tests have been supplemented and all issues raised are properly resolved.

Signed-off-by: aaron-ai <yangkun.ayk@alibaba-inc.com>

aaron-ai · 2020-04-23T02:04:06Z

All tests in CI are OK.

aaron-ai force-pushed the mq branch 5 times, most recently from f55ff0f to e7b43b6 Compare December 29, 2019 13:47

aaron-ai force-pushed the mq branch from e7b43b6 to 6973529 Compare December 30, 2019 02:50

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Jan 6, 2020

lizhanhui force-pushed the mq branch from fb457ff to eef367e Compare January 6, 2020 09:23

zyfjeff reviewed Jan 6, 2020

View reviewed changes

aaron-ai force-pushed the mq branch from eef367e to dc8a216 Compare January 6, 2020 09:41

mattklein123 assigned zyfjeff Jan 6, 2020

zyfjeff suggested changes Jan 6, 2020

View reviewed changes

aaron-ai force-pushed the mq branch from a061658 to 74ff5ba Compare January 7, 2020 02:09

dio reviewed Jan 7, 2020

View reviewed changes

zyfjeff suggested changes Jan 7, 2020

View reviewed changes

aaron-ai force-pushed the mq branch from 72bc666 to 0cce33f Compare January 7, 2020 05:57

lizhanhui force-pushed the mq branch from 4495806 to 9d50a6c Compare January 9, 2020 03:33

aaron-ai mentioned this pull request Jan 9, 2020

Delta CDS: add on-demand cds support #9626

Closed

aaron-ai force-pushed the mq branch 2 times, most recently from b9ed6ae to d719945 Compare January 9, 2020 09:48

zyfjeff suggested changes Jan 10, 2020

View reviewed changes

aaron-ai force-pushed the mq branch from f14a00f to 3c9d550 Compare January 11, 2020 06:06

zyfjeff suggested changes Mar 17, 2020

View reviewed changes

aaron-ai force-pushed the mq branch from cfc3d4a to e454b75 Compare March 23, 2020 06:45

zyfjeff previously approved these changes Mar 24, 2020

View reviewed changes

aaron-ai dismissed zyfjeff’s stale review via 05aa6fc March 27, 2020 14:53

aaron-ai force-pushed the mq branch from 4db29d0 to 05aa6fc Compare March 27, 2020 14:53

lizan added the no stalebot Disables stalebot from closing an issue label Apr 3, 2020

aaron-ai force-pushed the mq branch from e14694d to 4e9d3c1 Compare April 6, 2020 09:15

aaron-ai force-pushed the mq branch from 4e9d3c1 to 27b5caf Compare April 8, 2020 07:10

lizan reviewed Apr 12, 2020

View reviewed changes

lizan added waiting and removed no stalebot Disables stalebot from closing an issue labels Apr 12, 2020

aaron-ai force-pushed the mq branch from 27b5caf to a67d879 Compare April 15, 2020 13:36

repokitteh-read-only bot removed the waiting label Apr 15, 2020

aaron-ai force-pushed the mq branch 2 times, most recently from 65de232 to 0408438 Compare April 21, 2020 13:16

Add rocketmq's support

4131431

Signed-off-by: aaron-ai <yangkun.ayk@alibaba-inc.com>

aaron-ai force-pushed the mq branch from 0408438 to 4131431 Compare April 22, 2020 18:51

lizan approved these changes Apr 23, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 23, 2020

lizan merged commit 0a7dba7 into envoyproxy:master Apr 23, 2020


		TopicRouteData() = default;

		TopicRouteData(std::vector<QueueData>& queueData, std::vector<BrokerData>& brokerData)

rocketmq_proxy: implement rocketmq proxy #9503

rocketmq_proxy: implement rocketmq proxy #9503

Conversation

aaron-ai commented Dec 27, 2019 • edited

mattklein123 commented Dec 29, 2019

stale bot commented Jan 6, 2020

aaron-ai commented Jan 6, 2020

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyfjeff Jan 6, 2020 • edited

Choose a reason for hiding this comment

aaron-ai Jan 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Jan 7, 2020 • edited

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

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

aaron-ai commented Mar 6, 2020

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

aaron-ai commented Mar 24, 2020

zyfjeff left a comment

Choose a reason for hiding this comment

aaron-ai commented Mar 27, 2020

lizan commented Apr 3, 2020

aaron-ai commented Apr 3, 2020

htuch commented Apr 7, 2020

lizan left a 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

lizhanhui commented Apr 15, 2020

aaron-ai commented Apr 15, 2020

lizhanhui commented Apr 21, 2020

aaron-ai commented Apr 23, 2020

aaron-ai commented Dec 27, 2019 •

edited

zyfjeff Jan 6, 2020 •

edited

aaron-ai Jan 9, 2020 •

edited

dio Jan 7, 2020 •

edited