task_tracker: refactor on task_tracker #41

shengofsun · 2018-05-02T02:49:56Z

remove c interface of task tracker
remove task tracker from clientlet and fix all the
misusing of task tracker as it shouldn't be stored
in a base class

neverchanje · 2018-05-02T03:15:13Z

include/dsn/cpp/serverlet.h

@@ -159,8 +149,7 @@ class serverlet : public virtual clientlet

 // ------------- inline implementation ----------------
 template <typename T>
-inline serverlet<T>::serverlet(const char *nm, int task_bucket_count)
-    : clientlet(task_bucket_count), _name(nm)
+inline serverlet<T>::serverlet(const char *nm) : clientlet(), _name(nm)


默认构造函数就不需要写 clientlet() 了

neverchanje · 2018-05-03T06:26:53Z

src/tests/dsn/meta_state_service.cpp

@@ -224,7 +224,7 @@ void provider_recursively_create_delete_test(const service_creator_func &creator
                                             const service_deleter_func &deleter)
 {
    meta_state_service *service = creator();
-    clientlet tracker;
+    dsn::task_tracker tracker(1);


把 1 删掉

还不止一处，默认是1的地方都删掉吧

qinzuoyan · 2018-05-03T06:34:43Z

include/dsn/cpp/rpc_holder.h

-    task_ptr
-    call(::dsn::rpc_address server, clientlet *svc, TCallback &&callback, int reply_thread_hash = 0)
+    task_ptr call(::dsn::rpc_address server,
+                  dsn::task_tracker *svc,


变量名改一下，改成tracker

qinzuoyan · 2018-05-03T06:35:20Z

include/dsn/cpp/rpc_holder.h

@@ -273,7 +275,7 @@ namespace rpc {
 template <typename TCallback, typename TRpcHolder>
 task_ptr call(::dsn::rpc_address server,
              TRpcHolder rpc,
-              clientlet *svc,
+              dsn::task_tracker *svc,


变量名改一下，改成tracker

qinzuoyan · 2018-05-03T06:36:43Z

include/dsn/cpp/serverlet.h

@@ -117,7 +107,7 @@ template <typename T> // where T : serverlet<T>
 class serverlet : public virtual clientlet
 {
 public:
-    serverlet(const char *nm, int task_bucket_count = 1);
+    serverlet(const char *nm);


加explicit关键字

qinzuoyan · 2018-05-03T06:38:00Z

include/dsn/dist/failure_detector/failure_detector.h

@@ -218,6 +218,8 @@ class failure_detector : public failure_detector_service,
    mutable service::zlock _lock;
    // subClass can rewrite these method.
    virtual void send_beacon(::dsn::rpc_address node, uint64_t time);
+
+    dsn::task_tracker _tracker;


变量和_lock放在一起吧，别变量和方法混着放。

另外，在析构函数也需要调用 cancel_outstanding_tasks() ？

这个是在子类里面调用的

qinzuoyan · 2018-05-03T06:50:08Z

include/dsn/tool-api/task_tracker.h

-// task_tracker is the base class for RPC service and client
-// there can be multiple task_tracker in the system, mostly
-// defined during set_tracker in main
+// task_tracker mainly used to prevent a task from visit a deleted object. For example:


task_tracker is mainly used to prevent a task from visiting a deleted object.

qinzuoyan · 2018-05-03T07:25:52Z

src/dist/replication/meta_server/meta_service.h

@@ -193,6 +195,8 @@ class meta_service : public serverlet<meta_service>

    perf_counter_wrapper _recent_disconnect_count;
    perf_counter_wrapper _unalive_nodes_count;
+
+    dsn::task_tracker _tracker;


忘记在析构函数调用 cancel_outstanding_tasks() ？

qinzuoyan · 2018-05-03T07:29:14Z

src/dist/replication/meta_server/server_state.h

-    clientlet *tracker() { return &_tracker; }
-    void wait_all_task() { dsn_task_tracker_wait_all(_tracker.tracker()); }
+    task_tracker *tracker() { return &_tracker; }
+    void wait_all_task() { _tracker.wait_outstanding_tasks(); }


这两个方法没啥用，去掉吧

测试用的

qinzuoyan · 2018-05-03T07:30:04Z

src/dist/replication/meta_server/server_state.h

@@ -301,7 +302,7 @@ class server_state

    // ATTENTION:
    //  DO NOT use this tracker to track timer task
-    clientlet _tracker;
+    dsn::task_tracker _tracker;


这个ATTENTION分析一下，还有没有必要保留？如果有必要，把注释写详细写，为什么不能track timer task。

另外忘记在析构函数调用 cancel_outstanding_tasks() ？

qinzuoyan · 2018-05-03T07:32:16Z

src/dist/replication/zookeeper/meta_state_service_zookeeper.cpp

@@ -269,7 +266,7 @@ task_ptr meta_state_service_zookeeper::delete_node(const std::string &node,
            bind_and_enqueue(tsk, err);
        else if (children.empty())
            delete_empty_node(
-                node, cb_code, [tsk](error_code err) { bind_and_enqueue(tsk, err); }, this);
+                node, cb_code, [tsk](error_code err) { bind_and_enqueue(tsk, err); }, nullptr);


这里用null没有问题吗？

qinzuoyan · 2018-05-03T07:33:14Z

src/tests/dsn/meta_state_service.cpp

@@ -224,7 +224,7 @@ void provider_recursively_create_delete_test(const service_creator_func &creator
                                             const service_deleter_func &deleter)
 {
    meta_state_service *service = creator();
-    clientlet tracker;
+    dsn::task_tracker tracker(1);


还不止一处，默认是1的地方都删掉吧

shengofsun · 2018-05-04T04:00:12Z

已修改

qinzuoyan · 2018-05-04T07:16:15Z

src/apps/skv/simple_kv.app.example.h

        return ::dsn::ERR_OK;
    }

    virtual ::dsn::error_code stop(bool cleanup = false)
    {
+        _tracker.cancel_outstanding_tasks();


下面的 _timer->cancel(true); 是不是没必要了？

qinzuoyan · 2018-05-04T07:51:48Z

src/core/core/partition_resolver_simple.h

@@ -125,6 +125,8 @@ class partition_resolver_simple : public partition_resolver, public virtual clie
                            dsn_message_t request,
                            dsn_message_t response,
                            int partition_index);
+
+    dsn::task_tracker _tracker;


放在变量一起吧。
整理一下，把私有方法都放在一个单独的private段里面。
public:
// public methods
private:
// private methods
private:
// attributes

qinzuoyan · 2018-05-04T07:52:18Z

src/core/core/partition_resolver_simple.cpp

-partition_resolver_simple::~partition_resolver_simple() { clear_all_pending_requests(); }
+partition_resolver_simple::~partition_resolver_simple()
+{
+    _tracker.cancel_outstanding_tasks();


考虑下是放在clear_all_pending_requests()前面还是后面更合适

qinzuoyan · 2018-05-04T07:57:13Z

src/dist/replication/lib/replica.cpp

-        _checkpoint_timer->cancel(true);
-        _checkpoint_timer = nullptr;
-    }
+    // TODO: assert _checkpoint_timer.is_cancelled_or_finished


别TODO了，直接：
dassert(_checkpoint_timer->cancel(false), "checkpoint_timer should already been canceled");

qinzuoyan · 2018-05-04T07:59:49Z

src/dist/replication/lib/replica_chkpt.cpp

@@ -101,6 +99,7 @@ void replica::init_checkpoint(bool is_emergency)
        return;
    }

+    ::dsn::service::zauto_lock l(_async_checkpoint_lock);


如果用了tracker，就不需要 _async_checkpoint_task 和 _async_checkpoint_lock 变量了，相关代码都可以干掉

qinzuoyan · 2018-05-04T08:02:57Z

src/dist/replication/meta_server/server_state.h

@@ -300,8 +301,9 @@ class server_state
    friend class ::meta_service_test_app;

    // ATTENTION:
-    //  DO NOT use this tracker to track timer task
-    clientlet _tracker;
+    // when use tracker to track timer task, please make sure not to wait the timer task.


感觉这个注释放在 task_tracker::wait_outstanding_tasks() 那里更合适？

qinzuoyan · 2018-05-04T08:08:01Z

src/dist/replication/lib/mutation_log.h

@@ -339,6 +339,8 @@ class mutation_log : public ref_counter, public virtual clientlet
    int64_t _min_log_file_size_in_bytes;
    bool _force_flush;

+    dsn::task_tracker _tracker;


感觉在 mutation_log::close() 函数里面的 flush() 后面调用下 _tracker->cancel_outstanding_tasks() 更妥当：

// make all data is on disk flush(); _tracker->cancel_outstanding_tasks()

在flush的时候调过wait了

qinzuoyan · 2018-05-04T08:47:46Z

include/dsn/tool-api/task_tracker.h

+//   int value;
+//   task_tracker _tracker;
+// };
+//


直接写个Attention列表，包括不能wait timer task，都写到这里。

另外 so that to ensure all tasks.. 改为 to ensure that all tasks.. 比较好。

qinzuoyan

LGTM

neverchanje · 2018-05-07T02:48:50Z

include/dsn/cpp/clientlet.h

@@ -303,7 +295,7 @@ struct rpc_message_helper
    explicit rpc_message_helper(dsn_message_t request) : request(request) {}
    template <typename TCallback>
    task_ptr call(::dsn::rpc_address server,
-                  clientlet *owner,
+                  dsn::task_tracker *owner,


这里名字也改成 tracker ？

neverchanje · 2018-05-07T02:53:17Z

include/dsn/tool-api/task_tracker.h

+// task_tracker is used to track one ore more unfinished tasks, you may use it to
+// wait or cancel the tasks tracked by the tracker.
+//
+// A classical situation is use tracker to prevent a task from visiting a delete object.


deleted, not delete

neverchanje · 2018-05-07T02:57:17Z

include/dsn/tool/cli/cli.client.h

@@ -78,7 +78,7 @@ class cli_client : public virtual ::dsn::clientlet
        return ::dsn::rpc::call(server_addr.unwrap_or(_server),
                                RPC_CLI_CLI_CALL,
                                args,
-                                this,
+                                nullptr,


最好还是加上 tracker，方便以后可能的测试

测试的时候加？

1. remove c interface of task tracker 2. remove task tracker from clientlet and fix all the misusing of task tracker as it shouldn't be stored in a base class

qinzuoyan

LGTM

shengofsun requested review from qinzuoyan, neverchanje and LoveHeat May 2, 2018 02:50

neverchanje reviewed May 2, 2018

View reviewed changes

shengofsun force-pushed the master branch from e6f7e70 to be2e94c Compare May 2, 2018 06:38

neverchanje reviewed May 3, 2018

View reviewed changes

qinzuoyan reviewed May 3, 2018

View reviewed changes

shengofsun force-pushed the master branch from be2e94c to 5e41cf1 Compare May 4, 2018 03:57

qinzuoyan reviewed May 4, 2018

View reviewed changes

shengofsun force-pushed the master branch from 94abf68 to d91818b Compare May 4, 2018 10:50

qinzuoyan previously approved these changes May 7, 2018

View reviewed changes

neverchanje reviewed May 7, 2018

View reviewed changes

shengofsun added 3 commits May 7, 2018 11:55

task_tracker: refactor on task_tracker

d9c929a

1. remove c interface of task tracker 2. remove task tracker from clientlet and fix all the misusing of task tracker as it shouldn't be stored in a base class

task_tracker: add more comments

1c4fb49

task_tracker: fix based on comments

a7ca408

shengofsun dismissed qinzuoyan’s stale review via a7ca408 May 7, 2018 03:56

shengofsun force-pushed the master branch from 930b1ec to a7ca408 Compare May 7, 2018 03:56

replica: fix nullptr for test

62ad156

qinzuoyan approved these changes May 7, 2018

View reviewed changes

neverchanje approved these changes May 7, 2018

View reviewed changes

shengofsun merged commit f187c7b into XiaoMi:master May 7, 2018

qinzuoyan mentioned this pull request Aug 20, 2018

代码重构与化简 #141

Open

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

task_tracker: refactor on task_tracker #41

task_tracker: refactor on task_tracker #41

shengofsun commented May 2, 2018

neverchanje May 2, 2018

neverchanje May 3, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

shengofsun May 4, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

shengofsun May 4, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

qinzuoyan May 3, 2018

shengofsun commented May 4, 2018

qinzuoyan May 4, 2018

qinzuoyan May 4, 2018

qinzuoyan May 4, 2018

qinzuoyan May 4, 2018

qinzuoyan May 4, 2018

qinzuoyan May 4, 2018

qinzuoyan May 4, 2018

shengofsun May 4, 2018

qinzuoyan May 4, 2018

qinzuoyan left a comment

neverchanje May 7, 2018

shengofsun May 7, 2018

neverchanje May 7, 2018

shengofsun May 7, 2018

neverchanje May 7, 2018

shengofsun May 7, 2018

shengofsun May 7, 2018

qinzuoyan left a comment

task_tracker: refactor on task_tracker #41

task_tracker: refactor on task_tracker #41

Conversation

shengofsun commented May 2, 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

shengofsun commented May 4, 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

qinzuoyan 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

Choose a reason for hiding this comment

qinzuoyan left a comment

Choose a reason for hiding this comment