Henry Robinson has posted comments on this change. Change subject: IMPALA-3114: Add a pause monitor for impala processes. ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/2405/2/be/src/common/init.cc File be/src/common/init.cc: Line 17: #include <boost/thread/thread.hpp> please don't use boost::thread directly http://gerrit.cloudera.org:8080/#/c/2405/2/be/src/util/pause-monitor.cc File be/src/util/pause-monitor.cc: Line 25: monitor_thread_.reset(new boost::thread(&PauseMonitor::WorkLoop, this)); Please use our Thread API instead of boost::thread. Line 30: monitor_thread_->join(); Let's not join (or do any work) in the destructor. Why not just fail fast? Line 34: MonotonicStopWatch sw; just use MonotonicMillis() to measure time, and avoid the problem of having to convert to NS. Line 35: uint64_t extra_sleep_time; move this inside the loop http://gerrit.cloudera.org:8080/#/c/2405/2/be/src/util/pause-monitor.h File be/src/util/pause-monitor.h: Line 28: class PauseMonitor { I don't think you need a class for this. InitCommonRuntime() can start a thread running WorkLoop() directly. Line 39: milli milliseconds Line 46: /// Runs as long as stop is set to false. : bool stop; I don't think you need this coordination. Why wouldn't the pause monitor just run forever? -- To view, visit http://gerrit.cloudera.org:8080/2405 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I04eca63c0c44fa8f1b78833080acdc2176372263 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
