kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject incubator-kudu git commit: KUDU-1410 (part 2). rpc: clean up generated RPC service code
Date Wed, 20 Apr 2016 05:28:21 GMT
Repository: incubator-kudu
Updated Branches:
  refs/heads/master 6b6a697e4 -> 5c2a85a0c


KUDU-1410 (part 2). rpc: clean up generated RPC service code

This changes the generated RPC service code to extract the actual
handling logic into a non-generated base class. The generated
code now just generates a map of method name to MethodInfo, each
of which contains the requisite information to handle an RPC.

This results in a small reduction in lines of non-generated code,
and a larger reduction in lines of generated code.

Furthermore, this refactor makes it easier to make improvements and
further cleanups on the RPC server side, since most changes now
won't need to affect the code generator.

Change-Id: Id7b42a5ba1de59315660a79b125e70938c3a1b2e
Reviewed-on: http://gerrit.cloudera.org:8080/2795
Reviewed-by: Adar Dembo <adar@cloudera.com>
Tested-by: Kudu Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/5c2a85a0
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/5c2a85a0
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/5c2a85a0

Branch: refs/heads/master
Commit: 5c2a85a0c391db29da220251bdeb75a2f03caa02
Parents: 6b6a697
Author: Todd Lipcon <todd@apache.org>
Authored: Wed Apr 13 13:11:21 2016 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Wed Apr 20 05:28:12 2016 +0000

----------------------------------------------------------------------
 src/kudu/rpc/protoc-gen-krpc.cc | 107 +++++++++--------------------------
 src/kudu/rpc/service_if.cc      |  32 ++++++++---
 src/kudu/rpc/service_if.h       |  43 +++++++++++++-
 3 files changed, 92 insertions(+), 90 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/5c2a85a0/src/kudu/rpc/protoc-gen-krpc.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/protoc-gen-krpc.cc b/src/kudu/rpc/protoc-gen-krpc.cc
index bc6dd93..25690ee 100644
--- a/src/kudu/rpc/protoc-gen-krpc.cc
+++ b/src/kudu/rpc/protoc-gen-krpc.cc
@@ -326,6 +326,8 @@ class CodeGenerator : public ::google::protobuf::compiler::CodeGenerator
{
       "\n"
       "#include \"$path_no_extension$.pb.h\"\n"
       "\n"
+      "#include <functional>\n"
+      "#include <memory>\n"
       "#include <string>\n"
       "\n"
       "#include \"kudu/rpc/rpc_header.pb.h\"\n"
@@ -349,13 +351,11 @@ class CodeGenerator : public ::google::protobuf::compiler::CodeGenerator
{
       subs->PushService(service);
 
       Print(printer, *subs,
-        "\n"
-        "class $service_name$If : public ::kudu::rpc::ServiceIf {\n"
+        "class $service_name$If : public ::kudu::rpc::GeneratedServiceIf {\n"
         " public:\n"
         "  explicit $service_name$If(const scoped_refptr<MetricEntity>& entity);\n"
         "  virtual ~$service_name$If();\n"
-        "  virtual void Handle(::kudu::rpc::InboundCall *call);\n"
-        "  virtual std::string service_name() const;\n"
+        "  std::string service_name() const override;\n"
         "  static std::string static_service_name();\n"
         "\n"
         );
@@ -375,36 +375,6 @@ class CodeGenerator : public ::google::protobuf::compiler::CodeGenerator
{
 
       Print(printer, *subs,
         "\n"
-        " private:\n"
-      );
-
-
-      Print(printer, *subs,
-        "  enum RpcMetricIndexes {\n"
-      );
-      for (int method_idx = 0; method_idx < service->method_count();
-           ++method_idx) {
-        const MethodDescriptor *method = service->method(method_idx);
-        subs->PushMethod(method);
-
-        Print(printer, *subs,
-          "    $metric_enum_key$,\n"
-        );
-
-        subs->Pop();
-      }
-      Print(printer, *subs,
-        "  };\n" // enum
-      );
-
-      Print(printer, *subs,
-        "  static const int kMethodCount = $service_method_count$;\n"
-        "\n"
-        "  // Pre-initialize metrics because calling METRIC_foo.Instantiate() is expensive.\n"
-        "  void InitMetrics(const scoped_refptr<MetricEntity>& ent);\n"
-        "\n"
-        "  ::kudu::rpc::RpcMethodMetrics metrics_[kMethodCount];\n"
-        "\n"
         "};\n"
       );
 
@@ -459,6 +429,11 @@ class CodeGenerator : public ::google::protobuf::compiler::CodeGenerator
{
     }
 
     Print(printer, *subs,
+      "using google::protobuf::Message;\n"
+      "using kudu::rpc::RpcContext;\n"
+      "using kudu::rpc::RpcMethodInfo;\n"
+      "using std::unique_ptr;\n"
+      "\n"
       "$open_namespace$"
       "\n");
 
@@ -468,40 +443,33 @@ class CodeGenerator : public ::google::protobuf::compiler::CodeGenerator
{
       subs->PushService(service);
 
       Print(printer, *subs,
-        "$service_name$If::$service_name$If(const scoped_refptr<MetricEntity>&
entity) {\n"
-        "  InitMetrics(entity);\n"
-        "}\n"
-        "\n"
-        "$service_name$If::~$service_name$If() {\n"
-        "}\n"
-        "\n"
-        "void $service_name$If::Handle(::kudu::rpc::InboundCall *call) {\n"
-        "  {\n");
-
+        "$service_name$If::$service_name$If(const scoped_refptr<MetricEntity>&
entity) {\n");
       for (int method_idx = 0; method_idx < service->method_count();
            ++method_idx) {
         const MethodDescriptor *method = service->method(method_idx);
         subs->PushMethod(method);
 
         Print(printer, *subs,
-        "    if (call->remote_method().method_name() == \"$rpc_name$\") {\n"
-        "      $request$ *req = new $request$;\n"
-        "      if (PREDICT_FALSE(!ParseParam(call, req))) {\n"
-        "        delete req;\n"
-        "        return;\n"
-        "      }\n"
-        "      $response$ *resp = new $response$;\n"
-        "      $rpc_name$(req, resp,\n"
-        "          new ::kudu::rpc::RpcContext(call, req, resp,\n"
-        "                                      metrics_[$metric_enum_key$]));\n"
-        "      return;\n"
-        "    }\n"
-        "\n");
+              "  {\n"
+              "    unique_ptr<RpcMethodInfo> mi(new RpcMethodInfo());\n"
+              "    mi->req_prototype.reset(new $request$());\n"
+              "    mi->resp_prototype.reset(new $response$());\n"
+              "    mi->metrics.handler_latency =\n"
+              "        METRIC_handler_latency_$rpc_full_name_plainchars$.Instantiate(entity);\n"
+              "    mi->func = [this](const Message* req, Message* resp, RpcContext* ctx)
{\n"
+              "      this->$rpc_name$(static_cast<const $request$*>(req),\n"
+              "                       static_cast<$response$*>(resp),\n"
+              "                       ctx);\n"
+              "    };\n"
+              "    methods_by_name_[\"$rpc_name$\"] = std::move(mi);\n"
+              "  }\n");
         subs->Pop();
       }
+
       Print(printer, *subs,
-        "  }\n"
-        "  RespondBadMethod(call);\n"
+        "}\n"
+        "\n"
+        "$service_name$If::~$service_name$If() {\n"
         "}\n"
         "\n"
         "std::string $service_name$If::service_name() const {\n"
@@ -513,27 +481,6 @@ class CodeGenerator : public ::google::protobuf::compiler::CodeGenerator
{
         "\n"
       );
 
-      Print(printer, *subs,
-        "void $service_name$If::InitMetrics(const scoped_refptr<MetricEntity>&
entity) {\n"
-      );
-      // Expose per-RPC metrics.
-      for (int method_idx = 0; method_idx < service->method_count();
-           ++method_idx) {
-        const MethodDescriptor *method = service->method(method_idx);
-        subs->PushMethod(method);
-
-        Print(printer, *subs,
-          "  metrics_[$metric_enum_key$].handler_latency = \n"
-          "      METRIC_handler_latency_$rpc_full_name_plainchars$.Instantiate(entity);\n"
-        );
-
-        subs->Pop();
-      }
-      Print(printer, *subs,
-        "}\n"
-        "\n"
-      );
-
       subs->Pop();
     }
 

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/5c2a85a0/src/kudu/rpc/service_if.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/service_if.cc b/src/kudu/rpc/service_if.cc
index bbc9a2d..f78e9dd 100644
--- a/src/kudu/rpc/service_if.cc
+++ b/src/kudu/rpc/service_if.cc
@@ -17,27 +17,24 @@
 
 #include "kudu/rpc/service_if.h"
 
+#include <memory>
 #include <string>
 
 #include "kudu/gutil/strings/substitute.h"
 
 #include "kudu/rpc/connection.h"
 #include "kudu/rpc/inbound_call.h"
+#include "kudu/rpc/rpc_context.h"
 #include "kudu/rpc/rpc_header.pb.h"
 
+using google::protobuf::Message;
 using std::string;
+using std::unique_ptr;
 using strings::Substitute;
 
 namespace kudu {
 namespace rpc {
 
-RpcMethodMetrics::RpcMethodMetrics()
-  : handler_latency(nullptr) {
-}
-
-RpcMethodMetrics::~RpcMethodMetrics() {
-}
-
 ServiceIf::~ServiceIf() {
 }
 
@@ -78,5 +75,26 @@ void ServiceIf::RespondBadMethod(InboundCall *call) {
                        Status::InvalidArgument(err));
 }
 
+GeneratedServiceIf::~GeneratedServiceIf() {
+}
+
+
+void GeneratedServiceIf::Handle(InboundCall *call) {
+  const auto& method_name = call->remote_method().method_name();
+  const auto& it = methods_by_name_.find(method_name);
+  if (PREDICT_FALSE(it == methods_by_name_.end())) {
+    RespondBadMethod(call);
+    return;
+  }
+  const auto& method_info = it->second;
+  unique_ptr<Message> req(method_info->req_prototype->New());
+  if (PREDICT_FALSE(!ParseParam(call, req.get()))) {
+    return;
+  }
+  Message* resp = method_info->resp_prototype->New();
+  RpcContext* ctx = new RpcContext(call, req.get(), resp, method_info->metrics);
+  method_info->func(req.release(), resp, ctx);
+}
+
 } // namespace rpc
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/5c2a85a0/src/kudu/rpc/service_if.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/service_if.h b/src/kudu/rpc/service_if.h
index 83f49dd..a7ffa00 100644
--- a/src/kudu/rpc/service_if.h
+++ b/src/kudu/rpc/service_if.h
@@ -17,6 +17,7 @@
 #ifndef KUDU_RPC_SERVICE_IF_H
 #define KUDU_RPC_SERVICE_IF_H
 
+#include <unordered_map>
 #include <string>
 
 #include "kudu/gutil/macros.h"
@@ -37,14 +38,31 @@ class Histogram;
 namespace rpc {
 
 class InboundCall;
+class RpcContext;
 
 struct RpcMethodMetrics {
-  RpcMethodMetrics();
-  ~RpcMethodMetrics();
-
   scoped_refptr<Histogram> handler_latency;
 };
 
+// Generated services define an instance of this class for each
+// method that they implement. The generic server code implemented
+// by GeneratedServiceIf look up the RpcMethodInfo in order to handle
+// each RPC.
+struct RpcMethodInfo {
+  // Prototype protobufs for requests and responses.
+  // These are empty protobufs which are cloned in order to provide an
+  // instance for each request.
+  std::unique_ptr<google::protobuf::Message> req_prototype;
+  std::unique_ptr<google::protobuf::Message> resp_prototype;
+
+  RpcMethodMetrics metrics;
+
+  // The actual function to be called.
+  std::function<void(const google::protobuf::Message* req,
+                     google::protobuf::Message* resp,
+                     RpcContext* ctx)> func;
+};
+
 // Handles incoming messages that initiate an RPC.
 class ServiceIf {
  public:
@@ -60,7 +78,26 @@ class ServiceIf {
  protected:
   bool ParseParam(InboundCall* call, google::protobuf::Message* message);
   void RespondBadMethod(InboundCall* call);
+};
 
+
+// Base class for code-generated service classes.
+class GeneratedServiceIf : public ServiceIf {
+ public:
+  virtual ~GeneratedServiceIf();
+
+  // Looks up the appropriate method in 'methods_by_name_' and executes
+  // it on the current thread.
+  //
+  // If no such method is found, responds with an error.
+  void Handle(InboundCall* incoming) override;
+
+ protected:
+  // For each method, stores the relevant information about how to handle the
+  // call. Methods are inserted by the constructor of the generated subclass.
+  // After construction, this map is accessed by multiple threads and therefore
+  // must not be modified.
+  std::unordered_map<std::string, std::unique_ptr<RpcMethodInfo>> methods_by_name_;
 };
 
 } // namespace rpc


Mime
View raw message