Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18867 )
Change subject: [threadpool] Fix unsafe behaviour when SchedulerThread shutdown ...................................................................... Patch Set 16: (8 comments) http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG@9 PS16, Line 9: When shutdown thread pool with SchedulerThread nit: When shutting down a thread pool with SchedulerThread http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG@11 PS16, Line 11: schedule a task nit: a task is scheduled on the token http://gerrit.cloudera.org:8080/#/c/18867/16//COMMIT_MSG@14 PS16, Line 14: fix nit: fixes http://gerrit.cloudera.org:8080/#/c/18867/16/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: http://gerrit.cloudera.org:8080/#/c/18867/16/src/kudu/util/threadpool-test.cc@159 PS16, Line 159: SleepFor(MonoDelta::FromMilliseconds(2 * kDelayMs)); : pool_->Wait(); Is it essential to have both SleepFor() and pool_->Wait() here? Wouldn't just pool_-Wait() be enough? Please add a comment to clarify. http://gerrit.cloudera.org:8080/#/c/18867/16/src/kudu/util/threadpool-test.cc@256 PS16, Line 256: // Wait the schedule task execute. : SleepFor(MonoDelta::FromMilliseconds(200)); How essential is this extra wait? It seems there is pool_->Wait() below, so I'd guess that should be enough, no? Otherwise, please add a comment to explain why this is necessary. Also, with this extra wait, is the test stable enough in case of scheduling anomalies? E.g. try to run with --stress_cpu_threads=16 at an inferior hardware and see if it starts failing. http://gerrit.cloudera.org:8080/#/c/18867/16/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/18867/16/src/kudu/util/threadpool.h@187 PS16, Line 187: struct SchedulerTask { : ThreadPoolToken* thread_pool_token; : std::function<void()> f; : }; Is it possible to move this into the 'private' section of the class? If not, it would be great to document the fields. 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@680 PS13, Line 680: } > Yes, you are right. My origin intention is, users who use it, should know t This sounds like a good idea -- if you find this is needed elsewhere but not just in tests, you can introduce this or similar method back with proper API scope (e.g. this method is private/protected and used only in some higher-level wrapper method). Thanks! http://gerrit.cloudera.org:8080/#/c/18867/16/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/18867/16/src/kudu/util/threadpool.cc@540 PS16, Line 540: scheduler_->Schedule(token, std::move(f), execute_time); Should this be wrapped into RETURN_NOT_OK()? Maybe, just return scheduler_->Schedule(token, std::move(f), execute_time); -- 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: 16 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, 26 Sep 2022 01:38:49 +0000 Gerrit-HasComments: Yes
