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 14: (12 comments) http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@12 PS13, Line 12: trigge > nit: trigger Done http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@14 PS13, Line 14: refact > nit: refactor Done http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@126 PS13, Line 126: enable_sched > nit: how about "enable_scheduling" or somesuch Done http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@127 PS13, Line 127: scheduler_ > nit: precise_ms isn't particularly descriptive. How about "set_scheduler_pe Done http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@150 PS13, Line 150: ss Sched > nit: maybe I'm missing something -- I don't see any periodic scheduling hap Yes, you are right. I mean it can be used to implement dev/feature/timer_thread. I' ll fix the comments. http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@151 PS13, Line 151: : > nit: "timer" makes it seem like this should be measuring the time of tasks. OK http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@156 PS13, Line 156: // Start a thread to judge which tasks can be execute now. : Status Start(); : : // Shutdown the th > nit: please do your best to describe the behaviors of these methods in comm Done http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@168 PS13, Line 168: lock(mute > nit: I think "start_time" would be more descriptive Done http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@178 PS13, Line 178: : const std::string thread_pool_name_; : // timer's period che > These can be const Done http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@238 PS13, Line 238: enum class ScheduledTaskWaitType { : WAIT > nit: these names are not very descriptive. How about something like Done. http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@179 PS11, Line 179: const std::string thread_pool_name_; > Wouldn't it be better to name this variable period_ms_ (timer_period_ms_) o Done http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@142 PS13, Line 142: MutexLock auto_lock(mutex_); : while (!tasks_.empty()) { : auto it = tasks_.begin(); : if (it->first > now) { : break; : } : ThreadPoolToken* token = it->second.thread_pool_token_; : CHECK_OK(token->Submit(it->second.f)); : tasks_.erase(it); : task_size_--; : } : } : } > nit: I think it'd be a bit more understandable as 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: 14 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: Yuqi Du <[email protected]> Gerrit-Comment-Date: Sat, 07 May 2022 08:54:51 +0000 Gerrit-HasComments: Yes
