Sailesh Mukil 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 4: (26 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: virtual kudu::Status : QueueInboundCall(gscoped_ptr<kudu::rpc::InboundCall> call) OVERRIDE; : : const std::string service_name() const; : : private: : void RunThread(); : > let's add header function comments to these (and other methods), per usual. Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@69 PS3, Line 69: micInt > What is this referring to? RPC service methods, or something different? Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@72 PS3, Line 72: // appropriate internal KRPC state. Unused otherwise. > what is "time" in these cases? wallclock, cpu, ...? Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@74 PS3, Line 74: > what's that? some comments here would help. Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@92 PS3, Line 92: > what does that protect? Looks like it's left over from a long time ago in the Kudu code, so as far as we know it doesn't really protect anything any more. However, the Kudu folks were skeptical about removing it since it doesn't negatively affect anything right now, and we're not sure if there may be some issues after removing it. Let me know what you think. 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: if (PRED > Hmm, okay I see we are fishing into the timing_ struct and doing math again Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@186 PS3, Line 186: bably ig > internal is too vague. Let's "to update the InboundCall timing". timing() Done http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@187 PS3, Line 187: > I guess you're basically doing this manually via time_in_queue computation. This is the API that KRPC expects. They expect a Histogram to be supplied to update the incoming queue time for all RPCs. Handler latency is a per RPC histogram whereas the incoming queue time histogram is over all RPCs. In any case, I removed all per RPC histograms, so we're just passing the unused_histogram_ to satiate the API requirements now. http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@212 PS3, Line 212: > there's no enumeration for the methods? i guess this is okay though. Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@213 PS3, Line 213: > is that purposely protected by the lock? Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@220 PS3, Line 220: > this is a bit decieving becuase we haven't necessarily actually handled the Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@223 PS3, Line 223: > why not just do that? Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@240 PS3, Line 240: > doesn't krpc already have these histograms? is all we want to get out is th I have a working implementation of getting the histograms from Kudu and using it, however, since we decided to defer it, I'm not including it in this patch. Just to leave a note though, Kudu has the handling time histogram that we need to get from them since we cannot accurately calculate that ourselves. They payload size histogram and the per RPC queueing time histogram can still be calculated by us over here when we decide to do it. There are a few other interesting metrics like "total number of accepted connections", etc. that we could display. http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@255 PS3, Line 255: > should we also display the total number of service threads even though it's That's already displayed in the threadz/ page automatically, since all these threads are Impala threads. 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 It actually should be the same. Not sure why I removed that comment. I put it back in. 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: > delete Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.cc@138 PS3, Line 138: > shouldn't that be outbound? Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.cc@146 PS3, Line 146: > is there a danger this could run concurrently with RegisterService()? Removed this. N/A 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: rding the current > does this need updating? N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.h@129 PS3, Line 129: void InitRpcEventTracing(Webserver* webserver); > document the rpc_mgr parameter N/A 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: CK(even > nit: != NULL (or nullptr and adjust below) Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@91 PS3, Line 91: void RpcEventHandlerManager::JsonCallback(const Webserver::ArgumentMap& args, > I guess the whole point in integrating with this code is to share the /rpcz Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@95 PS3, Line 95: ent_handlers_ > should something happen for krpc stuff in this case? Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@100 PS3, Line 100: e > nit: != nullptr Removed this. N/A http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-trace.cc@122 PS3, Line 122: a > same Removed this. N/A 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: > couldn't we just do that in either case? Removed this. N/A -- 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: 4 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Sat, 02 Dec 2017 18:20:32 +0000 Gerrit-HasComments: Yes
