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: (4 comments) Was mid-review before the new revision, but the comments are still valid. 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@519 PS13, Line 519: if (load_meter_) { : MonoTime queue_head_submit_time; : if (!queue_.empty()) { : DCHECK(!queue_.front()->entries_.empty()); : queue_head_submit_time = queue_.front()->entries_.front().submit_time; : } : load_meter_->UpdateQueueInfoUnlocked(MonoDelta(), : queue_head_submit_time, : active_threads_ < max_threads_); : } : guard.Unlock(); : : if (metrics_.queue_length_histogram) { : metrics_.queue_length_histogram->Increment(length_at_submit); : } : if (token->metrics_.queue_length_histogram) { : Similar code is used in a few places now. Is it similar enough to encapsulate and reuse? 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) { Just making sure we have our bases covered, I think it's correct to say that we should call this whenever: - we dequeue/start a task, since we will end up with a new head (and thus a new queue head submit time) _and_ a new queue time to add to our history, - we complete a task, since we may end up with a new view of whether or not we have available threads, - we enqueue a new task, since this may be only item on the queue, and thus, may yield a new queue head submit time. Is that correct? http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@747 PS13, Line 747: Why don't we have to wait to decrement active_threads_ below? http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@885 PS13, Line 885: std::ostream& operator<<(std::ostream& o, ThreadPoolToken::State s Now that there is only a single caller, how about inlining this call instead? -- 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: Thu, 27 Aug 2020 22:53:42 +0000 Gerrit-HasComments: Yes
