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 14: (3 comments) http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.h@368 PS14, Line 368: spawn nit: spawned, here and below http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@667 PS13, Line 667: nst int64_t queue_time_us = queue_time.ToMicroseconds(); : TRACE_COUNTER_INCREMENT(queue_time_trace_metric_name_, queue_time_us); : if (metrics_.queue_time_us_histogram) { > Right, UpdateQueueInfoUnlocked should report about events which affects our Great, thanks for clarifying. Do you think it's worth mentioning that somewhere in a comment? http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/16332/14/src/kudu/util/threadpool.cc@723 PS14, Line 723: load_meter_->UpdateQueueInfoUnlocked(MonoDelta(), MonoTime(), true); Other call-sites can't assume these arguments just because the queue is empty. But we can here, because not only do we know the queue is empty, but we know there is an available thread (the current thread), also don't have an update to the queue time history. Mind adding a comment here explaining that? Future readers may come upon this code and wonder why the difference in calls. That said, could we have reused the same code block in all three call-sites? -- 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: 14 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: Fri, 28 Aug 2020 02:53:28 +0000 Gerrit-HasComments: Yes
