Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool ...................................................................... Patch Set 17: (9 comments) http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@126 PS17, Line 126: ThreadPoolBuilder& set_enable_scheduler(bool enable_scheduler); The default value is false, `set_enable_scheduler` to true implicit, the parameter is not needed? http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@127 PS17, Line 127: set_scheduler_period_ms nit: schedule_period_ms? http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@187 PS17, Line 187: mutable Mutex mutex_; Nit: add comments to describe which variables 'mutex_' to protect. http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@579 PS17, Line 579: uint32_t scheduler_period_ms_; nit: schedule_period_ms_ ? http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@114 PS17, Line 114: std::string nit: string http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@141 PS17, Line 141: MutexLock auto_lock(mutex_); To reduce lock time, you can obtain and erase the tasks at first, then submit them out of the lock. And maybe multimap.lower_bound maybe used to optimize a little. http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@243 PS17, Line 243: CHECK(mode() == ThreadPool::ExecutionMode::SERIAL); nit: CHECK_EQ http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@393 PS17, Line 393: scheduler_->Start(); Check if it is OK. http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@633 PS17, Line 633: // set ScheduledTaskWaitType::WAIT at unit tests. If ScheduledTaskWaitType::WAIT is only used in tests, better to add a new private function for it. enum class ScheduledTaskWaitType also in private. -- To view, visit http://gerrit.cloudera.org:8080/18447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e Gerrit-Change-Number: 18447 Gerrit-PatchSet: 17 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 10 May 2022 09:32:36 +0000 Gerrit-HasComments: Yes
