Tim Armstrong has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 2: (10 comments) I had some initial comments. Need to digest a bit more but I thought I'd push out the comments so you knew there would be some conflicts with my patch. http://gerrit.cloudera.org:8080/#/c/7730/1//COMMIT_MSG Commit Message: Line 38: 3. The Thread::Create and ThreadPool::Create have Would unique_ptr do the job? I think you will need to support unique_ptr anyway once you rebase since ThreadGroup::AddThread() now takes a unique_ptr. http://gerrit.cloudera.org:8080/#/c/7730/2//COMMIT_MSG Commit Message: Line 18: 1. util/thread.h's Thread constructor is now private I think it's a matter of taste, but in some ways I prefer the constructor + Init() approach because it gives the caller control over how the object is managed. The Create() approach seems to work out fine though. Line 34: 1. QueryState::StartFInstances() needs appropriate One option is to defer the tricky cases til a follow-up patch and bring down the process with LOG(FATAL) or similar, since that's still a strict improvement on the status quo. http://gerrit.cloudera.org:8080/#/c/7730/1/be/src/exec/blocking-join-node.cc File be/src/exec/blocking-join-node.cc: Line 197: RETURN_IF_ERROR(Thread::Create(FragmentInstanceState::FINST_THREAD_GROUP_NAME, Do we need to release the thread token on this error path? http://gerrit.cloudera.org:8080/#/c/7730/1/be/src/runtime/parallel-executor.cc File be/src/runtime/parallel-executor.cc: Line 39: RETURN_IF_ERROR(Thread::Create("parallel-executor", ss.str(), If we created some threads I think we still need to call JoinAll() to avoid leaving a bunch of orphaned threads. http://gerrit.cloudera.org:8080/#/c/7730/1/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: Line 339: // TODO: This error handling needs work. I think worst-case we could do a LOG(FATAL) for now and come back to fix this in a follow-up patch. That would still be easier to diagnose than the current behaviour. http://gerrit.cloudera.org:8080/#/c/7730/1/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: PS1, Line 74: (void) U should use discard_result() once you rebase onto my change (GCC 7 does not consider (void) as actually discarding the status). http://gerrit.cloudera.org:8080/#/c/7730/1/be/src/statestore/statestore.h File be/src/statestore/statestore.h: Line 404: boost::scoped_ptr<ThreadPool<ScheduledSubscriberUpdate>> subscriber_topic_update_threadpool_; long lines http://gerrit.cloudera.org:8080/#/c/7730/1/be/src/util/thread-pool.h File be/src/util/thread-pool.h: Line 123: Status CreateThreads(const std::string& group, const std::string& thread_prefix, Should add WARN_UNUSED_RESULT to make sure it's not dropped anywhere. Line 185: static Status Create(const std::string& group, const std::string& thread_prefix, Should add WARN_UNUSED_RESULT to make sure it's not dropped anywhere. -- 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
