Alexey Serbin 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 Done 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 Good catch: it turned out this is not needed as a part of internal state of the object. 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 thoug Right, this 'if' condition is very unlikely to evaluate to 'true' in case of a pool that handles mostly tasks in CONCURRENT execution mode, but in general case it might be more or less regular case. However, 'load_meter_' is used only in case of apply pool, and that's why I added this PREDICT_FALSE(). I agree that it's better to remove PREDICT_FALSE() here because the code doesn't make any explicit assumptions on the way how this functionality is used. 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 Nope, I don't think it make sense to put this atop of the method. The first check is to see whether the queue was overloaded upon the last update on the queue from the threadpool's activity. If that's the case, there is no need to dig further. However, there might be no updates on the queue for a long time, and the task in the head of the queue might be sitting there for longer than the configured queue_time_threshold_ interval. There is an explanation in the comment above, maybe it's not clear enough. I'll add more comments; hopefully it's clearer now. -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 24 Aug 2020 17:39:59 +0000 Gerrit-HasComments: Yes
