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, 01 Sep 2017 18:49:48 GMT
Sailesh Mukil has posted comments on this change.

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


Patch Set 8:

(8 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.


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 173: scanner_threads_.AddThread(move(t));
> Here is my understanding:
Good point, I was looking at the base code for Close() instead of your updated version where
you added SetDone(). Looks like you may have fixed this previously existing subtle race by
adding that.


PS7, Line 240: SetDoneInternal();
We previously didn't call materialized_row_batches_->Shutdown() here, but now we will,
is that acceptable?


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.


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 one of the other
headers anyway)


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 now. I just want
to confirm if that's okay.


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


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


-- 
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