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: (8 comments) 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 Hmm, okay I see we are fishing into the timing_ struct and doing math against those values, so let's leave this computation using MonoTime. However, how about instead of doing our own Now(), shouldn't we move this computation down and write it in terms of timing() fields? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@186 PS3, Line 186: internal internal is too vague. Let's "to update the InboundCall timing". timing() is a method on InboundCall so we can talk about it. 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? I guess you're basically doing this manually via time_in_queue computation. But the handler_latency_histogram is inside a method_info_ thing that they allow to be null. Why isn't this histogram also in there? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@212 PS3, Line 212: method_name there's no enumeration for the methods? i guess this is okay though. http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@213 PS3, Line 213: method->num_in_handlers.Add(1); is that purposely protected by the lock? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@220 PS3, Line 220: uint64_t time_to_handle = (MonoTime::Now() - monotime_after_dequeue).ToMilliseconds(); this is a bit decieving becuase we haven't necessarily actually handled the RPC at this point. It may be deferred. Do we have something equivalent to the method_info_->handler_latency_histogram? http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@223 PS3, Line 223: // rid of the spin lock. why not just do that? 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 the json representation? how hard would it be to just use the kudu ones and get the json out of those? are there other histograms/metrics that Kudu has that would be useful to expose? if so it seems like duplicating them like this will be a pain. -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Wed, 15 Nov 2017 20:35:57 +0000 Gerrit-HasComments: Yes
