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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
