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 12: (7 comments) Thanks your crs. 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? No. 'pool_->scheduler()' the method has removed. add it back. http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@188 PS11, Line 188: a random time: 1ms or 2ms or 3ms. > 1 and 3 are magic numbers here, what do they mean? could you add some comme ok, it;s a random delay time from 1, 2, 3. http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@195 PS11, Line 195: > nit: how about Done http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@202 PS11, Line 202: SERT_OK(RebuildPoolWithScheduler(1, 1, 1)); : unique_ptr<ThreadPoolToken> token = pool_->NewToken(ThreadPool::ExecutionMode::SERIAL); : for (int i = 0; i < 2048; i++) { : ContinuousIssueTraceStatement(token.get()); : } : SleepFor(MonoDelta::FromMilliseconds(5)); : pool_->Shutdown(); > Duplicate code with function ContinuousIssueTraceStatement? Is it possible Done http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool-test.cc@236 PS11, Line 236: > Is it OK to schedule this task even though the token is shutdown? according to syntax, It should not ok. Fix it. 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: > In ThreadPool::WaitForScheduler, 'scheduler_' is also used for some purpose Should protect it. http://gerrit.cloudera.org:8080/#/c/18867/11/src/kudu/util/threadpool.cc@418 PS11, Line 418: > nit: In L530, the guard is using a simple name 'l', it suggest to use the s done -- 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: 12 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: Tue, 13 Sep 2022 07:05:07 +0000 Gerrit-HasComments: Yes
