impala-reviews mailing list archives

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

(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

Mime
View raw message