[GitHub] [incubator-doris] acelyc111 commented on a change in pull request #5654: [Optimize] Make tablet meta checkpoint to be threadpool model

[GitBox <gi...@apache.org>]

acelyc111 commented on a change in pull request #5654:
URL: https://github.com/apache/incubator-doris/pull/5654#discussion_r613178145



##########
File path: be/src/common/config.h
##########
@@ -286,6 +286,10 @@ CONF_mInt64(min_compaction_failure_interval_sec, "600"); // 10 min
 CONF_mInt32(min_compaction_threads, "10");
 CONF_mInt32(max_compaction_threads, "10");
 
+// This config can be set to limit thread number in tablet meta checkpoint thread pool.
+CONF_mInt32(min_meta_checkpoint_threads, "10");
+CONF_mInt32(max_meta_checkpoint_threads, "10");

Review comment:
       Thread count is mot mutable, use CONF_Int32 instead, and we can just set a MAX thread count, leave MIN thread count as default value which is 0. BTW, you can optimize compaction_threads like this.
   And how about set the default value to -1, which means use the data directory count as the thread count, which is the previous behavior?

##########
File path: be/src/olap/olap_server.cpp
##########
@@ -81,16 +81,18 @@ Status StorageEngine::start_bg_threads() {
             &_compaction_tasks_producer_thread));
     LOG(INFO) << "compaction tasks producer thread started";
 
-    // tablet checkpoint thread
-    for (auto data_dir : data_dirs) {
-        scoped_refptr<Thread> tablet_checkpoint_thread;
-        RETURN_IF_ERROR(Thread::create(
-                "StorageEngine", "tablet_checkpoint_thread",
-                [this, data_dir]() { this->_tablet_checkpoint_callback(data_dir); },
-                &tablet_checkpoint_thread));
-        _tablet_checkpoint_threads.emplace_back(tablet_checkpoint_thread);
-    }
-    LOG(INFO) << "tablet checkpoint thread started";
+    int32_t max_checkpoint_thread_num = config::max_meta_checkpoint_threads;
+    int32_t min_checkpoint_thread_num = config::min_meta_checkpoint_threads;
+    ThreadPoolBuilder("TabletMetaCheckpointTaskThreadPool")
+            .set_min_threads(min_checkpoint_thread_num)
+            .set_max_threads(max_checkpoint_thread_num)
+            .build(&_tablet_meta_checkpoint_thread_pool);

Review comment:
       _tablet_meta_checkpoint_thread_pool need shutdown in deconstructor.

##########
File path: be/src/olap/storage_engine.h
##########
@@ -313,8 +313,8 @@ class StorageEngine {
     std::vector<scoped_refptr<Thread>> _path_gc_threads;
     // threads to scan disk paths
     std::vector<scoped_refptr<Thread>> _path_scan_threads;
-    // threads to run tablet checkpoint
-    std::vector<scoped_refptr<Thread>> _tablet_checkpoint_threads;
+    // threads to produce tablet checkpoint tasks

Review comment:
       ```suggestion
       // thread to produce tablet checkpoint tasks
   ```

##########
File path: be/src/common/config.h
##########
@@ -516,6 +520,7 @@ CONF_Int32(flush_thread_num_per_store, "2");
 // config for tablet meta checkpoint
 CONF_mInt32(tablet_meta_checkpoint_min_new_rowsets_num, "10");
 CONF_mInt32(tablet_meta_checkpoint_min_interval_secs, "600");
+CONF_mInt32(generate_tablet_meta_checkpoint_tasks_interval_secs, "10");

Review comment:
       Would produce checkpoint task every 10 seconds cause BE do checkpoint task too frequently? better to keep the behavior similar as before to set it to 600?

##########
File path: be/src/olap/olap_server.cpp
##########
@@ -290,21 +292,19 @@ void StorageEngine::_path_scan_thread_callback(DataDir* data_dir) {
     } while (!_stop_background_threads_latch.wait_for(MonoDelta::FromSeconds(interval)));
 }
 
-void StorageEngine::_tablet_checkpoint_callback(DataDir* data_dir) {
+void StorageEngine::_tablet_checkpoint_callback(std::vector<DataDir*> data_dirs) {
 #ifdef GOOGLE_PROFILER
     ProfilerRegisterThread();
 #endif
 
-    int64_t interval = config::tablet_meta_checkpoint_min_interval_secs;
+    int64_t interval = config::generate_tablet_meta_checkpoint_tasks_interval_secs;
     do {
-        LOG(INFO) << "begin to do tablet meta checkpoint:" << data_dir->path();
-        int64_t start_time = UnixMillis();
-        _tablet_manager->do_tablet_meta_checkpoint(data_dir);
-        int64_t used_time = (UnixMillis() - start_time) / 1000;
-        if (used_time < config::tablet_meta_checkpoint_min_interval_secs) {
-            interval = config::tablet_meta_checkpoint_min_interval_secs - used_time;
-        } else {
-            interval = 1;
+        LOG(INFO) << "begin to produce tablet meta checkpoint tasks.";
+        for (auto data_dir : data_dirs) {
+            auto st =_tablet_meta_checkpoint_thread_pool->submit_func([=]() {
+                CgroupsMgr::apply_system_cgroup();
+                _tablet_manager->do_tablet_meta_checkpoint(data_dir);
+            });
         }

Review comment:
       Could you keep the interval config mutable as before?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org