Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708182317.v7INHvf6020097@ip-10-146-233-104.ec2.internal>
Date: Fri, 18 Aug 2017 23:17:57 +0000
From: "Sailesh Mukil (Code Review)" <gerrit@cloudera.org>
To: Joe McDonnell <joemcdonnell@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>
Reply-To: sailesh@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: Fri, 18 Aug 2017 23:18:03 -0000

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