Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool ...................................................................... Patch Set 18: (9 comments) Thanks your crs. 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(); > The default value is false, `set_enable_scheduler` to true implicit, the pa Done http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@127 PS17, Line 127: chedule_period_ms); > nit: schedule_period_ms? Done http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@187 PS17, Line 187: // Protect `tasks_` data race. > Nit: add comments to describe which variables 'mutex_' to protect. Done http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.h@579 PS17, Line 579: uint32_t schedule_period_ms_; > nit: schedule_period_ms_ ? Done 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: > nit: string Done http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@141 PS17, Line 141: MonoTime now = MonoTime::Now(); > To reduce lock time, you can obtain and erase the tasks at first, then subm I have try to reduce the lock time. lower_bound maybe not good, because task equal MonoTime should be executed, the bound may exclude some tasks. http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@243 PS17, Line 243: } > nit: CHECK_EQ CHECK_EQ not ok. ThreadPool::ExecutionMode should printable, but no implements. So reserve it. http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@393 PS17, Line 393: Status status = CreateThread(); > Check if it is OK. Done http://gerrit.cloudera.org:8080/#/c/18447/17/src/kudu/util/threadpool.cc@633 PS17, Line 633: > If ScheduledTaskWaitType::WAIT is only used in tests, better to add a new p Done -- 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: 18 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: Wed, 11 May 2022 09:04:51 +0000 Gerrit-HasComments: Yes
