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 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h File src/kudu/util/threadpool.h: http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@75 PS2, Line 75: queue_time_history_lenght > nit: typo Done http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@79 PS2, Line 79: set_queue_time_history_length > Where is this defined? This has gone. I updated the comment. http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@99 PS2, Line 99: // the information on the next task's submit time is specified : // via the 'queue_head_submit_time' parameter. > nit: can you also mention that it is OK for queue_head_submit_time to be un Done http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.h@108 PS2, Line 108: overloaded > nit: unfinished sentence? Done http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc File src/kudu/util/threadpool.cc: http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@70 PS2, Line 70: VLOG(4) << Substitute("XXX"); > nit: update these to be actual messages Done http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@132 PS2, Line 132: if (queue_head_submit_time.Initialized()) { : queue_head_submit_time_ = queue_head_submit_time; : } > Why don't we want to call this if 'queue_head_submit_time' is uninitialized Ah, indeed. It seems I changed the semantics of this in-the-middle, and that's the old one. Good catch! http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@138 PS2, Line 138: UpdateQueueInfo > nit: also suffix this and TaskDequeued() with Unlocked? Done http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@710 PS2, Line 710: true > nit: annotate with /*has_idle_threads*/ Done http://gerrit.cloudera.org:8080/#/c/16332/2/src/kudu/util/threadpool.cc@756 PS2, Line 756: // Prepare information for load_meter. : ThreadPoolToken* head_token = nullptr; : // Find the element to be dispatched next. For tokens with execution mode : // CONCURRENT, the next task is referred from a token in the head of queue_, : // but for tokens of the SERIAL execution mode, the token isn't in the : // queue_, but will be put there after executing current task. : if (!queue_.empty() && !queue_.front()->entries_.empty()) { : head_token = queue_.front(); : } else if (!token->entries_.empty()) { : head_token = token; : } > Can we move this into the if(load_meter) block? Done -- 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: 2 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: Sat, 15 Aug 2020 02:41:14 +0000 Gerrit-HasComments: Yes
