Khazar Mammadli has posted comments on this change. ( http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool ...................................................................... Patch Set 11: (6 comments) 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: // timer's period checking time. Wouldn't it be better to name this variable period_ms_ (timer_period_ms_) or something similar, so the user could understand the use case of the variable from a glance instead of looking for the specific comment? http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@571 PS11, Line 571: // TimerThread is used for some scenioes, such as Nit: Scenarios http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@572 PS11, Line 572: // delay execute is schedule, Nit: scheduled http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@573 PS11, Line 573: // another thread send request. Comment is rather unclear, could you please clarify it a bit? http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.h@602 PS11, Line 602: // Submit a task, running after delay_ms delay some time Does this run the task submitted and delay afterwards or introduces a delay before the task starts the execution? Or is it a periodic delay, aka execute first task--->delay--->execute second task--->delay? http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/18447/11/src/kudu/util/threadpool.cc@247 PS11, Line 247: return Status::NotSupported(Substitute("not support mode: CONCURRENT")); nit: Unsupported mode: CONCURRENT -- 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: 11 Gerrit-Owner: Yuqi Du <[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: Fri, 06 May 2022 09:20:57 +0000 Gerrit-HasComments: Yes
