Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16332 )
Change subject: KUDU-1587 part 1: load meter for ThreadPool ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool-test.cc File src/kudu/util/threadpool-test.cc: http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool-test.cc@191 PS7, Line 191: then nit: than http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.h@336 PS7, Line 336: bool Does this need to be atomic? If not, maybe mention that this should only be accessed when the threadpool's lock is held? http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc@608 PS7, Line 608: if (PREDICT_FALSE(active_threads_ Just checking I understand the nuances of why this is important. Even though the queue may be empty, another thread may be processing work from a currently-running token, which isn't reflected by the queue's emptiness. Therefore, we need to check the active thread count before determining that there is indeed no work currently being processed. Otherwise, more entries will be dequeued from the other thread's token which would update the queue overloading info, but before doing so, we'd incorrectly have temporarily marked the queue as not overloaded. If so, I'm curious just how rare that actually is (i.e. why PREDICT_FALSE?). It seems like the big places we use SERIAL are: - in each tablet's prepare pool token - in each tablet's Raft pool token Why is it more likely that the number of active threads will be zero when the queue is empty? I may be missing something, but If there are many tokens doing work concurrently, I'd think the number active threads upon emptying 'queue_' would be non-zero a lot of the time. http://gerrit.cloudera.org:8080/#/c/16332/7/src/kudu/util/threadpool.cc@809 PS7, Line 809: MonoTime queue_head_submit_time = queue_head_submit_time_.load(); : if (!queue_head_submit_time.Initialized()) { : return false; : } I'm curious why do this _after_ checking whether the meter has been deemed not previously overloaded. Would it make sense to put this at the top of the method? -- To view, visit http://gerrit.cloudera.org:8080/16332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I640716dc32f193e68361ca623ee7b9271e661d8b Gerrit-Change-Number: 16332 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 17 Aug 2020 23:35:38 +0000 Gerrit-HasComments: Yes
