impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
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. ( )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala

Patch Set 3:

File be/src/rpc/impala-service-pool.h:
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.
PS3, Line 69: Method
What is this referring to? RPC service methods, or something different?
PS3, Line 72:     std::unique_ptr<HistogramMetric> handling_time;
what is "time" in these cases? wallclock, cpu, ...?
PS3, Line 74:     AtomicInt32 num_in_handlers;
what's that? some comments here would help.
PS3, Line 92:   boost::mutex shutdown_lock_;
what does that protect?
File be/src/rpc/
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?
PS3, Line 186:     // We need to call RecordHandlingStarted() to update some internal InboundCall
That seems weird. why is that?
PS3, Line 187: unused_histogram_
there's nothing useful to get out of this thing?
PS3, Line 207: 
note to self: finish here
File be/src/rpc/rpc-mgr.h:
PS3, Line 160: 
is that not still true? i.e. since we need to inherit from kudu::RpcService
File be/src/rpc/
PS3, Line 129: this->
PS3, Line 138: inbound
shouldn't that be outbound?

let's make sure we test this change somehow.
PS3, Line 146:   for (auto service_pool : service_pools_) {
is there a danger this could run concurrently with RegisterService()?
File be/src/rpc/rpc-trace.h:
PS3, Line 38: per ThriftServer.
does this need updating?
PS3, Line 129: /// Initialises rpc event tracing, must be called before any RpcEventHandlers
are created.
document the rpc_mgr parameter
File be/src/rpc/
PS3, Line 86: rpc_mgr
nit: != NULL (or nullptr and adjust below)
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.
PS3, Line 95: "/rpcz_reset"
should something happen for krpc stuff in this case?
PS3, Line 100: )
nit: != nullptr
PS3, Line 122: )
File be/src/service/
PS3, Line 86:     InitRpcEventTracing(exec_env.webserver(), exec_env.rpc_mgr());
couldn't we just do that in either case?

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Comment-Date: Wed, 15 Nov 2017 00:00:00 +0000
Gerrit-HasComments: Yes

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