Todd Lipcon has posted comments on this change. Change subject: KUDU-1444. Get resource usage of a scan. ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/3013/1/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: Line 781: TRACE_COUNTER_INCREMENT(TraceMetrics::InternName("bytes_read_from_disk"), data_block.size()); here you don't need to InternName, because the name itself is a string constant -- it will just end up costing extra CPU to look it up in the intern dictionary every time. Also, we already have the cfile_cache_miss_bytes and cfile_cache_hit_bytes counters incremented in CFileReader::ReadBlock, so can we just re-use those? I wouldn't be surprised if callers were actually interested in distinguishing between the two from a statistics standpoint. http://gerrit.cloudera.org:8080/#/c/3013/1/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 1093: if (data_->last_response_.has_resource_usage()) { instead of duplicating the code, could you increment it at the same place where you get the RPC response? I think SendScanRpc is shared between both the 'Open' and the later call http://gerrit.cloudera.org:8080/#/c/3013/1/src/kudu/client/client.h File src/kudu/client/client.h: Line 1011: // Returns this scanner's current resource usage. this should be clear that it's the cumulative resource usage since the scan was started. http://gerrit.cloudera.org:8080/#/c/3013/1/src/kudu/client/resource_usage.h File src/kudu/client/resource_usage.h: Line 40: int64_t bytes_read_from_disk_; since this is part of the public API, it needs to be PIMPLed. I'm wondering whether it makes more sense to just expose a map<string, long> type API instead, so we could pass back _all_ the trace metrics from a query, including stuff like spinlock contention, timed blocked on locks, etc -- those things are equally interesting from a performance troubleshooting perspective, even if they aren't as useful for billing/quotas. http://gerrit.cloudera.org:8080/#/c/3013/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 1083: context->trace()->metrics()->GetMetric(TraceMetrics::InternName("bytes_read_from_disk"))); instead of using InternName, I think you should declare this constant somewhere in a .cc file and refer to it here. http://gerrit.cloudera.org:8080/#/c/3013/1/src/kudu/util/trace_metrics.h File src/kudu/util/trace_metrics.h: Line 58: // Return metric's current value. should document here that it's important that the 'const char* name' provided is compared pointer-wise with the metrics in the map, not string-compared, so requires that the same const char* be used for the lookup as was used for the insertion. Line 79: lock_guard<simple_spinlock> l(&lock_); nit: indentation in this function is messy Line 85: } above 5 lines or so could just use FindWithDefault -- To view, visit http://gerrit.cloudera.org:8080/3013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedaf570a7601651c93275ae0a8565f1e33da842d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
