Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18867 )
Change subject: [threadpool] Fix unsafe behaviour when SchedulerThread shutdown ...................................................................... Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@a289 PS11, Line 289: Now this check will fail? http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@188 PS11, Line 188: 1 + (static_cast<int>(current_time) % 3) 1 and 3 are magic numbers here, what do they mean? could you add some comments? http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@195 PS11, Line 195: In SchedulerThread have many task need to scheduled nit: how about There are many tasks in SchedulerThread need to be scheduled http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@202 PS11, Line 202: time_t current_time = time(nullptr); : int delay_ms = 1 + (static_cast<int>(current_time) % 3); : Status status = : token->Schedule(std::bind(&ContinuousIssueTraceStatement, token.get(), i), delay_ms); : // At the case, 'pool_->Shutdown();' would shutdown its SchedulerThread firstly, after that : // 'token->Scheduler(...)' would return a IllegalState. : ASSERT_TRUE(status.ok() || status.IsIllegalState()); Duplicate code with function ContinuousIssueTraceStatement? Is it possible to improve the design to reduce duplicate code when we use 'Scheduler'? For example, the task submit to the token can be auto re-submit when it complete? http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@236 PS11, Line 236: ASSERT_OK(token->Schedule(&IssueTraceStatement, 1000)); Is it OK to schedule this task even though the token is shutdown? http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool.cc@418 PS11, Line 418: MutexLock unique_lock(scheduler_lock_); In ThreadPool::WaitForScheduler, 'scheduler_' is also used for some purpose, why not protect it? http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool.cc@418 PS11, Line 418: unique_lock nit: In L530, the guard is using a simple name 'l', it suggest to use the same name for the same lock. -- 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: 11 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, 05 Sep 2022 16:37:07 +0000 Gerrit-HasComments: Yes
