Tim Armstrong has posted comments on this change. Change subject: IMPALA-3114: Add a pause monitor for impala processes. ......................................................................
Patch Set 2: (9 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 really need them. I think this goes away if we get rid of that thread anyway. http://gerrit.cloudera.org:8080/#/c/2405/2/be/src/common/init.cc File be/src/common/init.cc: Line 79: shared_ptr can be scoped_ptr I think. I'm not sure why maintenance_thread is a shared_ptr. Line 80: static void MaintenanceThread() { Add extra space before function start (formatting was weird before your change) 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 for this change: http://gerrit.cloudera.org:8080/#/c/2105/ It would be good to keep the log message though in case ReleaseFreeMemory() takes a long time. 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's useful for debugging, but if it is, it should just be a debug-level log message. http://gerrit.cloudera.org:8080/#/c/2405/2/be/src/util/pause-monitor.cc File be/src/util/pause-monitor.cc: Line 36: stop The C++ standard doesn't guarantee that changes to the variable by other threads will be seen by this case. I think we need some kind of memory barrier. It might be adequate to declare stop as volatile, but it's probably easiest to reason about if we make stop an atomic variable. http://gerrit.cloudera.org:8080/#/c/2405/2/be/src/util/pause-monitor.h File be/src/util/pause-monitor.h: Line 30: /// Starts a new pause monitor thread. This is indented 1 space too many. Line 31: PauseMonitor We should change the interface so that the constructor doesn't start the thread. Rather we should have a separate Start() method that returns status. Sailesh is working on some changes to thread error handling that will require this. Line 35: private: Additional line before private: -- 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
