Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8472 )
Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads ...................................................................... Patch Set 3: (21 comments) http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@52 PS3, Line 52: kudu::rpc::RpcMethodInfo* LookupMethod(const kudu::rpc::RemoteMethod& method) override; : : virtual kudu::Status : QueueInboundCall(gscoped_ptr<kudu::rpc::InboundCall> call) OVERRIDE; : : const std::string service_name() const; : : let's add header function comments to these (and other methods), per usual. http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@69 PS3, Line 69: Method What is this referring to? RPC service methods, or something different? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@72 PS3, Line 72: std::unique_ptr<HistogramMetric> handling_time; what is "time" in these cases? wallclock, cpu, ...? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@74 PS3, Line 74: AtomicInt32 num_in_handlers; what's that? some comments here would help. http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@92 PS3, Line 92: boost::mutex shutdown_lock_; what does that protect? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@182 PS3, Line 182: MonoTime how about using impala util/time.h routines so we don't have to read about another set of time functions? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@186 PS3, Line 186: // We need to call RecordHandlingStarted() to update some internal InboundCall state. That seems weird. why is that? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@187 PS3, Line 187: unused_histogram_ there's nothing useful to get out of this thing? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@207 PS3, Line 207: note to self: finish here http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.h@a160 PS3, Line 160: is that not still true? i.e. since we need to inherit from kudu::RpcService http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.cc@129 PS3, Line 129: this-> delete http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.cc@138 PS3, Line 138: inbound shouldn't that be outbound? let's make sure we test this change somehow. http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.cc@146 PS3, Line 146: for (auto service_pool : service_pools_) { is there a danger this could run concurrently with RegisterService()? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.h File be/src/rpc/rpc-trace.h: http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.h@38 PS3, Line 38: per ThriftServer. does this need updating? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.h@129 PS3, Line 129: /// Initialises rpc event tracing, must be called before any RpcEventHandlers are created. document the rpc_mgr parameter http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@86 PS3, Line 86: rpc_mgr nit: != NULL (or nullptr and adjust below) http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@91 PS3, Line 91: webserver->RegisterUrlCallback("/rpcz", "rpcz.tmpl", json); I guess the whole point in integrating with this code is to share the /rpcz page? if that's the case, how about being more explicit about that? or rather, that the point of InitRpcEventTracing() is to register callbacks for these paths. http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@95 PS3, Line 95: "/rpcz_reset" should something happen for krpc stuff in this case? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@100 PS3, Line 100: ) nit: != nullptr http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@122 PS3, Line 122: ) same http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/service/impalad-main.cc@86 PS3, Line 86: InitRpcEventTracing(exec_env.webserver(), exec_env.rpc_mgr()); couldn't we just do that in either case? -- To view, visit http://gerrit.cloudera.org:8080/8472 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448 Gerrit-Change-Number: 8472 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Comment-Date: Wed, 15 Nov 2017 00:00:00 +0000 Gerrit-HasComments: Yes
