Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9690 )
Change subject: IMPALA-6685: Improve profiles in KrpcDataStreamRecvr and KrpcDataStreamSender ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/9690/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9690/1//COMMIT_MSG@48 PS1, Line 48: DeferredBatches Following our in-person discussion, this should probably be cummulative. http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h File be/src/runtime/krpc-data-stream-recvr.h: http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@240 PS1, Line 240: RuntimeProfile::Counter* deserialize_row_batch_timer_; > it would be helpful to distinguish which counters belong to which profile. Should we group them in separate structs? That (or encoding in the names) has the benefit of showing it at the call-sites, too. http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@263 PS1, Line 263: /// Total wall-clock time of row batches spent in deferred queue before being processed. : RuntimeProfile::Counter* deferred_queue_time_; > Instead of tracking the cumulative time of RPCs spent in the deferred queue Should we collect a full histogram instead? You can use a HdrHistogram to track the time and print is using HistogramMetric::HistogramToHumanReadable(). http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.h@292 PS1, Line 292: RuntimeProfile::Counter* producer_wait_time_; maybe also add unit here and to the other _time_ members? http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc@461 PS1, Line 461: COUNTER_ADD(recvr_->deferred_queue_time_, deferred_duration); Should this be a histogram, too? http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc@604 PS1, Line 604: receiver and sender Are these the other way round? I think receiver is the enqueue profile, sender is the dequeue? http://gerrit.cloudera.org:8080/#/c/9690/1/be/src/runtime/krpc-data-stream-recvr.cc@639 PS1, Line 639: TotalBatchesDeferred Should we have a separate counter for the early sender batches? -- To view, visit http://gerrit.cloudera.org:8080/9690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ba405921b3df920c1e85b940ce9c8d02fc647cd Gerrit-Change-Number: 9690 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Comment-Date: Tue, 20 Mar 2018 17:29:48 +0000 Gerrit-HasComments: Yes
