impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads
Date Wed, 15 Nov 2017 00:00:00 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 00:00:00 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message