impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Fri, 18 Aug 2017 22:50:53 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5750: Catch exceptions from boost thread creation

Patch Set 2:


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.
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.
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.
File be/src/exec/

Line 197:     RETURN_IF_ERROR(Thread::Create(FragmentInstanceState::FINST_THREAD_GROUP_NAME,
Do we need to release the thread token on this error path?
File be/src/runtime/

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.
File be/src/runtime/

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.
File be/src/service/

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).
File be/src/statestore/statestore.h:

Line 404:   boost::scoped_ptr<ThreadPool<ScheduledSubscriberUpdate>> subscriber_topic_update_threadpool_;
long lines
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message