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 13: (3 comments) > (4 comments) > > Was mid-review before the new revision, but the comments are still > valid. Sorry for the mess. I was trying to remove spurious updates on spare thread availability, and in PS14 it looks simpler now. 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; : } : QueueLoadMeter::SpareThreadAvailability spare_thread_presence = : QueueLoadMeter::SpareThreadAvailability::UNKNOWN; : if (!idle_threads_.empty()) { : spare_thread_presence = QueueLoadMeter::SpareThreadAvailability::PRESENT; : } else if (max_threads_ == active_threads_) { : spare_thread_presence = QueueLoadMeter::SpareThreadAvailability::ABSENT; : } : load_meter_->UpdateQueueInfoUnlocked(MonoDelta(), : queue_head_submit_time, : spare_thread_presence); : } > Similar code is used in a few places now. Is it similar enough to encapsula I simplified these, and in PS14 it's not that much of the code. If talking about PS14, does it still look like a valid concern? http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@667 PS13, Line 667: load_meter_->UpdateQueueInfoUnlocked(queue_time, : queue_head_submit_time, : spare_thread_presence); > Just making sure we have our bases covered, I think it's correct to say tha Right, UpdateQueueInfoUnlocked should report about events which affects our criterion to evaluate the state of the queue (overloaded vs normal). These are updates about: * queue time of a newly dequeued task * submit time of the task at the head of the queue * change in the availability of spare working threads It means we want to call UpdateQueueInfoUnlocked() at each of the following events: * the task at the head of the queue dispatched to be run * a new task scheduled (i.e. added into the queue) * a worker thread completes running a task http://gerrit.cloudera.org:8080/#/c/16332/13/src/kudu/util/threadpool.cc@747 PS13, Line 747: max_threads_ == active_threads_ > Why don't we have to wait to decrement active_threads_ below? This outer 'else' cause goes next cycle of the outer while loop and picks up a next task immediately, incrementing active_threads_ back (while holding lock). Instead, the idea was to avoid such spurious reports. -- 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: 13 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 00:32:54 +0000 Gerrit-HasComments: Yes
