impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joe McDonnell (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Fri, 01 Sep 2017 23:34:51 GMT
Joe McDonnell has posted comments on this change.

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


Patch Set 8:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/catalog/catalog-server.h
File be/src/catalog/catalog-server.h:

PS8, Line 39: class TGetAllCatalogObjectsResponse;
> nit: Not your change, but no longer required.
Done


http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/exec/kudu-scan-node.cc
File be/src/exec/kudu-scan-node.cc:

PS7, Line 240: SetDoneInternal();
> We previously didn't call materialized_row_batches_->Shutdown() here, but n
This previously called Shutdown() when the last scanner thread is done.

I looked into this, and there doesn't seem to be any reason to wait. It is safe to call Shutdown()
when the other scanner threads are still around. We do that for the HDFS version of this.
We also do it if we reach a limit. I looked at the different methods on the queue, and I don't
see any problem.


http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:

PS8, Line 22: #include <boost/scoped_ptr.hpp>
> No longer needed.
Done


http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:

PS8, Line 40: throw atc::SystemResourceException("Thread::Create() failed");
> is it worth incorporating the status msg into the exception message?
I think that makes sense. The status has the category and thread name, which could be useful
to us.


http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/runtime/disk-io-mgr-handle-cache.h
File be/src/runtime/disk-io-mgr-handle-cache.h:

Line 28: #include "common/hdfs.h"
> nit: Include what you use, status.h (it's already being pulled in through o
Done


http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

PS8, Line 341: admission_controller_->Init()
> The admission controller dequeue thread starts a little later than before n
I think it is ok. ExecEnv::StartServices() is called from impalad-main.cc soon after constructing
the ExecEnv and before starting the backend or beeswax or hs2 servers. There should be nothing
to admit.

One thing I noticed is that StartServices() is not called from FeSupport. I think that should
be ok, but I'm checking.


http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/runtime/thread-resource-mgr.h
File be/src/runtime/thread-resource-mgr.h:

Line 288:   if (!in_callback) InvokeCallbacks();
> This explains why we need to skip the callbacks. But why is it safe to do s
You're right. This is only safe from a callback if a thread did TryAcquireThreadToken and
then wants to return it without using it. This correct usage is equivalent to just passing
on picking up a thread token. If the callback did a release of an unrelated thread, then that
would be a problem, because we won't find a replacement for that thread. There is no real
way to tell if something has called TryAcquireThreadToken. We don't give it an object or anything.

I can augment the comment to detail that it must be used in a very targeted way, or there
could be separate functions.


http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/service/child-query.h
File be/src/service/child-query.h:

PS8, Line 22: <boost/thread.hpp>
> boost/thread/mutex.hpp
Done


http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

Line 30: 
> nit: include what you use: status.h
Done


-- 
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@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

Mime
View raw message