Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708211839.v7LIdT13022014@ip-10-146-233-104.ec2.internal>
Date: Mon, 21 Aug 2017 18:39:29 +0000
From: "Joe McDonnell (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Sailesh Mukil <sailesh@cloudera.com>,
        Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: joemcdonnell@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5750=3A_Catch_exceptions_from_boost_thread_creation=0A?=
In-Reply-To: <gerrit.1503094817000.I15a2f278dc71892b7fec09593f81b1a57ab725c0@gerrit.cloudera.org>
References: <gerrit.1503094817000.I15a2f278dc71892b7fec09593f81b1a57ab725c0@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Mon, 21 Aug 2017 18:39:35 -0000

Joe McDonnell has posted comments on this change.

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


Patch Set 1:

(17 comments)

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 an
Changed to 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 +
Yes, the constructor + Init() also makes subclassing easier. I switched ThreadPool over to this approach. I think it makes more sense for that. Thread seems like either would work, so I'm sticking with a static factory method for now.


Line 34: 1. QueryState::StartFInstances() needs appropriate
> One option is to defer the tricky cases til a follow-up patch and bring dow
I did that for query-state.cc, but I think that is probably the most common place for new threads to hit this exception, so it would be really nice to get it working.


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?
Good point, this definitely needs to release the thread token. Fixed.


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 ex
Setting stop_=true means we will not execute the loop and instead will do shutdown cleanup in the "if (stop_)" block and exit the function. Exiting the function stops the server.

At various points in this file, we catch exceptions, so I think we don't want an exception.


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
Yes, that definitely needs to happen. Those threads reference stack variables, so it would be very dangerous to just leave. Changed to do the JoinAll().


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 th
Changed to LOG(FATAL) for the moment, but I am still looking into getting this to work.


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 
Done


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
Done


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:
Done


PS2, Line 72: StartMemoryMaintenanceThread();
> ABORT_IF_ERROR(StartMemoryMaintenanceThread());
Done


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.
Done


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.
Done


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.
Changed approach on ThreadPool to make it follow the constructor + Init() pattern, so this change is gone.


PS2, Line 184: /// TODO: add comment
> Comment?
Removed


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?
Switching to the constructor + Init() pattern eliminated the need for this code. Now CallableThreadPool is the same as before.


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' 
This code has been removed.


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes