Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 2: (8 comments) Did an initial pass. http://gerrit.cloudera.org:8080/#/c/7730/1//COMMIT_MSG Commit Message: Line 8: nit: You can make it up to 72 chars per line to make this more readable. http://gerrit.cloudera.org:8080/#/c/7730/2/be/src/rpc/TAcceptQueueServer.cpp File be/src/rpc/TAcceptQueueServer.cpp: Line 226: stop_ = true; Should we throw an exception here? What's the behavior when it continues executing? http://gerrit.cloudera.org:8080/#/c/7730/2/be/src/statestore/statestored-main.cc File be/src/statestore/statestored-main.cc: Line 71: ABORT_IF_ERROR(RegisterMemoryMetrics(metrics.get(), false, nullptr, nullptr)); You also need to do: ABORT_IF_ERROR(StartMemoryMaintenanceThread()); in impalad-main.cc and catalogd-main.cc PS2, Line 72: StartMemoryMaintenanceThread(); ABORT_IF_ERROR(StartMemoryMaintenanceThread()); http://gerrit.cloudera.org:8080/#/c/7730/2/be/src/util/thread-pool.h File be/src/util/thread-pool.h: Line 46: /// -- work_function: the function to run every time an item is consumed from the queue Update comment to include 'tpool' arg. PS2, Line 184: /// TODO: add comment Comment? PS2, Line 188: DCHECK(pool == nullptr); : boost::scoped_ptr<CallableThreadPool> local_tpool; : local_tpool.reset(new CallableThreadPool(queue_size, &CallableThreadPool::Worker)); : RETURN_IF_ERROR(local_tpool->CreateThreads(group, thread_prefix, num_threads)); : pool.swap(local_tpool); : return Status::OK(); Why not just call the parent's Create() here? i.e. ThreadPool::Create() http://gerrit.cloudera.org:8080/#/c/7730/2/be/src/util/thread.cc File be/src/util/thread.cc: PS2, Line 319: local_thread Just call 't' for consistency with the above StartThread(). 'local_thread' sounds a bit confusing. -- To view, visit http://gerrit.cloudera.org:8080/7730 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
