impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(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 <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message