rpc: several refactors for rpc #27

shengofsun · 2018-04-23T08:46:52Z

remove c interface of register rpc handler
remove rpc code of RPC_L2_CLIENT_READ/WRITE, replace with proper storage engine rpc code.
move some storage engine rpc_code's properties from config file to source code: is_write_operation, allow_batch, thread_pool

shengofsun · 2018-04-23T09:09:05Z

include/dsn/c/api_layer1.h

-                                             void *context,
-                                             dsn::gpid gpid DEFAULT(dsn::gpid()));
+                                             const char *extra_name,
+                                             const dsn_rpc_request_handler_t &cb);

 /*! unregister callback to handle RPC request, and returns void* context upon \ref


这个注释我需要改

neverchanje · 2018-04-23T09:37:29Z

include/dsn/dist/replication/replication.codes.h

@@ -44,6 +44,11 @@ DEFINE_THREAD_POOL_CODE(THREAD_POOL_LOCAL_APP)
 DEFINE_THREAD_POOL_CODE(THREAD_POOL_REPLICATION_LONG)
 DEFINE_THREAD_POOL_CODE(THREAD_POOL_COMPACT)

+#define STORAGE_WRITE(x, allow_batch)                                                              \


qinzuoyan · 2018-04-24T02:39:39Z

include/dsn/tool-api/task_code.h

@@ -122,7 +129,7 @@ class task_code
    int _internal_code{0};
 };

-// you can define task_cods by the following macros
+// you can define task_codes by the following macros


task_codes -> task_code

qinzuoyan · 2018-04-24T02:40:57Z

include/dsn/tool-api/task_code.h

+#define DEFINE_STORAGE_RPC_CODE(x, pri, pool, is_write, allow_batch)                               \
+    __selectany const ::dsn::task_code x(                                                          \
+        #x, TASK_TYPE_RPC_REQUEST, pri, pool, is_write, allow_batch);                              \
+    __selectany const ::dsn::task_code x##_ACK(                                                    \


为什么Reponse用THREAD_POOL_DEFAULT？说明原因并加注释

用THREAD_POOL_DEFAULT没问题，不过这里还是用注释解释清楚，免得以后再看又要想

qinzuoyan · 2018-04-24T02:55:55Z

src/core/core/rpc_engine.h

+        dsn_rpc_request_handler_t h;
+    };
+
+    typedef std::unordered_map<std::string, handler_entry *> rpc_handlers;


是否用 std::unordered_map<std::string, std::unique_ptr<handler_entry>> 更好？

handler_entry是两个地方都共享，用uniq_ptr没有让代码更简单。要用就用shared_ptr, 或者用我找找有没有ref counted thread unsafe的公共类

qinzuoyan · 2018-04-24T02:56:55Z

src/core/core/rpc_engine.h

    rpc_handlers _handlers;
    mutable utils::rw_lock_nr _handlers_lock;

-    std::vector<std::pair<rpc_handler_info *, utils::rw_lock_nr> *> _vhandlers;
+    std::vector<std::pair<handler_entry *, utils::rw_lock_nr> *> _vhandlers;


这里为何用pair*来着？可以直接用 std::vector<std::pair<handler_entry *, utils::rw_lock_nr>> 吗？

在vector里面初始化utils::rw_lock_nr比较麻烦，各种编译不过……

那么pair里面的handler_entry *至少可以改为 std::unique<handler_entry>吧？

这个改了也不好，因为hander_entry是被_handlers这个map指向了两次，然后又被_vhandler指向了一次。有三个reference

qinzuoyan · 2018-04-24T03:02:52Z

include/dsn/cpp/serverlet.h

-    dsn_rpc_request_handler_t cb = [](dsn_message_t request, void *param) {
-        auto hc2 = (hc_type1 *)param;
-
+    dsn_rpc_request_handler_t cb = [this, handler](dsn_message_t request) {


是不是可以不赋值，而是直接放到函数参数里面？

对下层来说太麻烦了，一下要支持好多种类型的handler, rpc_engine的register接口就是一个std::function<void (dsn_message_t)> 来说会清晰非常多。性能问题应该还好，this和handler都是个单纯的指针

qinzuoyan · 2018-04-24T03:05:03Z

src/core/core/rpc_engine.h

@@ -140,8 +148,10 @@ class rpc_engine
    //
    // rpc registrations
    //
-    bool register_rpc_handler(rpc_handler_info *handler);
-    rpc_handler_info *unregister_rpc_handler(dsn::task_code rpc_code);
+    bool register_rpc_handler(dsn::task_code code,


是不是可以增加参数为 dsn_rpc_request_handler_t &&h 的重载函数？

反正注册就是个一锤子买卖，就算拷贝也就拷贝一次。

qinzuoyan · 2018-04-24T03:05:25Z

include/dsn/c/api_layer1.h

-                                             void *context,
-                                             dsn::gpid gpid DEFAULT(dsn::gpid()));
+                                             const char *extra_name,
+                                             const dsn_rpc_request_handler_t &cb);


是不是可以增加 dsn_rpc_request_handler_t &&h 的重载函数？

qinzuoyan · 2018-04-24T03:23:18Z

include/dsn/tool-api/task_spec.h

 public:
    // not configurable [
    dsn::task_code code;
    dsn_task_type_t type;
    std::string name;
    dsn::task_code rpc_paired_code;
    shared_exp_delay rpc_request_delayer;
+
+    bool rpc_request_for_storage;


没有初始化？

待会加一下

qinzuoyan · 2018-04-24T10:05:30Z

src/core/core/rpc_engine.cpp

 {
-    std::string name(handler->code.to_string());
+    handler_entry *ctx = new handler_entry{code, extra_name, h};


handler_entry在哪里delete的？

在unregister_rpc_handler的时候delete的

如果没有调用 unregister_rpc_handler，析构函数也没有delete？

qinzuoyan · 2018-04-24T10:06:57Z

src/core/core/rpc_engine.h

    rpc_handlers _handlers;
    mutable utils::rw_lock_nr _handlers_lock;

-    std::vector<std::pair<rpc_handler_info *, utils::rw_lock_nr> *> _vhandlers;
+    std::vector<std::pair<handler_entry *, utils::rw_lock_nr> *> _vhandlers;


那么pair里面的handler_entry *至少可以改为 std::unique<handler_entry>吧？

qinzuoyan · 2018-04-24T10:07:56Z

include/dsn/tool-api/task_code.h

+#define DEFINE_STORAGE_RPC_CODE(x, pri, pool, is_write, allow_batch)                               \
+    __selectany const ::dsn::task_code x(                                                          \
+        #x, TASK_TYPE_RPC_REQUEST, pri, pool, is_write, allow_batch);                              \
+    __selectany const ::dsn::task_code x##_ACK(                                                    \


用THREAD_POOL_DEFAULT没问题，不过这里还是用注释解释清楚，免得以后再看又要想

shengofsun requested review from qinzuoyan, LoveHeat and neverchanje April 23, 2018 08:47

shengofsun commented Apr 23, 2018

View reviewed changes

neverchanje reviewed Apr 23, 2018

View reviewed changes

refactor: remove c interface of register rpc handler

8cb8dee

shengofsun force-pushed the master branch from 09fbf6a to 8cb8dee Compare April 23, 2018 13:16

qinzuoyan reviewed Apr 24, 2018

View reviewed changes

rpc: fix based on comments

31250b8

qinzuoyan reviewed Apr 24, 2018

View reviewed changes

rpc: fix rpc based on comments

cea61f4

qinzuoyan approved these changes Apr 25, 2018

View reviewed changes

shengofsun merged commit 0d35598 into XiaoMi:master Apr 25, 2018

qinzuoyan mentioned this pull request Aug 20, 2018

代码重构与化简 #141

Open

28 tasks

foreverneverer mentioned this pull request Jan 10, 2020

ASAN: heap-buffer-overflow in meta_test_base.h #376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: several refactors for rpc #27

rpc: several refactors for rpc #27

shengofsun commented Apr 23, 2018

shengofsun Apr 23, 2018

neverchanje Apr 23, 2018

qinzuoyan Apr 24, 2018

qinzuoyan Apr 24, 2018

qinzuoyan Apr 24, 2018

qinzuoyan Apr 24, 2018

shengofsun Apr 24, 2018

qinzuoyan Apr 24, 2018

shengofsun Apr 24, 2018

qinzuoyan Apr 24, 2018

shengofsun Apr 24, 2018

qinzuoyan Apr 24, 2018

shengofsun Apr 24, 2018

qinzuoyan Apr 24, 2018

shengofsun Apr 24, 2018

qinzuoyan Apr 24, 2018

qinzuoyan Apr 24, 2018

shengofsun Apr 24, 2018

qinzuoyan Apr 24, 2018

shengofsun Apr 24, 2018

qinzuoyan Apr 24, 2018

qinzuoyan Apr 24, 2018

qinzuoyan Apr 24, 2018

rpc: several refactors for rpc #27

rpc: several refactors for rpc #27

Conversation

shengofsun commented Apr 23, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment