Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18867 )
Change subject: [threadpool] Fix unsafe behaviour when SchedulerThread shutdown ...................................................................... Patch Set 13: (8 comments) http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@187 PS13, Line 187: struct SchedulerTask { : ThreadPoolToken* thread_pool_token; : std::function<void()> f; : }; > Is this structure really needs to be public? I just use a simple struct, no other reasons. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@192 PS13, Line 192: void Schedule(ThreadPoolToken* token, : std::function<void()> f, : const MonoTime& execute_time) { > Please document this public method as well: what it does and how the parame token can be null. Add a ut case. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@276 PS13, Line 276: Status Schedule(ThreadPoolToken* token, std::function<void()> f, MonoTime execute_time); > Please follow the pattern for other methods in this interface and add a con Comments have been added. token can be null. In class 'ThreadPool', I want to protect ‘scheduler_’ using a mutexlock. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@506 PS13, Line 506: void WaitForScheduler(); > Where is this method used in addition to the ThreadPoolTest.TestSimpleTasks Have Removed. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.h@608 PS13, Line 608: // Protect 'scheduler_'. > Since WaitForScheduler() acquires another lock along with 'scheduler_lock_' 'WaitForScheduler' is removed. deadlock is impossible. The two lock cann't locked at the same time. http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc@678 PS13, Line 678: pending tasks finish > for pending tasks to finish Done http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc@678 PS13, Line 678: , for example : // at unit tests. > I'm not sure this reference to unit tests is quite relevant here. Maybe, a Done http://gerrit.cloudera.org:8080/#/c/18867/13/src/kudu/util/threadpool.cc@680 PS13, Line 680: MutexLock l(scheduler_lock_); > From what I can see in the code, ThreadPool::WaitForScheduler() isn't used Yes, you are right. My origin intention is, users who use it, should know the tasks will finish to avoid take a long time to wait tasks's finish. Just like my unittest cases. But this is hard. So, deleting it is better. -- To view, visit http://gerrit.cloudera.org:8080/18867 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I021422f7e51e1007c5bdbc877ab445f70ba12357 Gerrit-Change-Number: 18867 Gerrit-PatchSet: 13 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Mon, 19 Sep 2022 11:40:36 +0000 Gerrit-HasComments: Yes
