impala-reviews mailing list archives

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

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

Patch Set 2:


Did an initial pass.
Commit Message:

Line 8: 
nit: You can make it up to 72 chars per line  to make this more readable.
File be/src/rpc/TAcceptQueueServer.cpp:

Line 226:     stop_ = true;
Should we throw an exception here? What's the behavior when it continues executing?
File be/src/statestore/

Line 71:   ABORT_IF_ERROR(RegisterMemoryMetrics(metrics.get(), false, nullptr, nullptr));
You also need to do:

in and

PS2, Line 72: StartMemoryMaintenanceThread();
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

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()
File be/src/util/

PS2, Line 319: local_thread
Just call 't' for consistency with the above StartThread(). 'local_thread' sounds a bit confusing.

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: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message