Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-3114: Add a pause monitor for impala processes. ......................................................................
Patch Set 2: (18 comments) http://gerrit.cloudera.org:8080/#/c/2405/2/CMakeLists.txt File CMakeLists.txt: Line 140: chrono > I think it's best to avoid pulling in extra boost dependencies unless we re Done 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 Done Line 79: shared_ptr > can be scoped_ptr I think. I'm not sure why maintenance_thread is a shared_ Yea I thought about it too. Making it a scoped_ptr. Line 80: static void MaintenanceThread() { > Add extra space before function start (formatting was weird before your cha Done Line 116: if (!free_memory.try_join_for(boost::chrono::seconds(5))) { > Let's not create the extra thread here, we have a different fix in flight f Yep, just saw the CR. makes sense to remove this. I don't think we need to add an error message here as the other patch seems to be logging something if the call time exceeds TcmallocUtil::LOG_THRESHOLD_MS Line 203: LOG(INFO) << "Pause monitor started"; > I don't think we need this log message at INFO level. I'm not sure that it' Refactored the code. Removed this. 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. Done Line 30: monitor_thread_->join(); > Let's not join (or do any work) in the destructor. Why not just fail fast? Agreed. I can't think of a case on why someone would want to stop this. Line 34: MonotonicStopWatch sw; > just use MonotonicMillis() to measure time, and avoid the problem of having Great. Done Line 35: uint64_t extra_sleep_time; > move this inside the loop Done Line 36: stop > The C++ standard doesn't guarantee that changes to the variable by other th Refactored the code. Based on Henry's suggestion, made this an infinite 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 th I thought of design but then due to "stopping" functionality, I ended creating a separate class for this. However I made this an infinite loop, so removed this and added a static fn in init Line 30: /// Starts a new pause monitor thread. > This is indented 1 space too many. Done Line 31: PauseMonitor > We should change the interface so that the constructor doesn't start the th Done Line 35: private: > Additional line before private: Done Line 39: milli > milliseconds Done Line 44: static const uint64_t WARN_THRESHOLD_NS = 10000 * 1000000UL; > it may be better to make this configurable. or even disable. Not really sure why would someone want to disable this? 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 ju Agreed. Done. -- 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
