Todd Lipcon has posted comments on this change. Change subject: KUDU-1444. Get resource metrics of a scan. ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/3013/2/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: Line 57: const char* cfile_cache_miss_bytes = "cfile_cache_miss_bytes"; style: name something like kCFileCacheMissBytesMetricName or CFILE_CACHE_MISS_BYTES_METRIC_NAME Line 58: const char* cfile_cache_hit_bytes = "cfile_cache_hit_bytes"; same http://gerrit.cloudera.org:8080/#/c/3013/2/src/kudu/client/resource_metrics-internal.h File src/kudu/client/resource_metrics-internal.h: Line 45: mutable simple_spinlock lock_; nit: move spinlock above counters_ http://gerrit.cloudera.org:8080/#/c/3013/2/src/kudu/client/resource_metrics.cc File src/kudu/client/resource_metrics.cc: Line 18: #include "resource_metrics.h" kudu/client/ http://gerrit.cloudera.org:8080/#/c/3013/2/src/kudu/client/resource_metrics.h File src/kudu/client/resource_metrics.h: Line 26: nit: remove blank line here http://gerrit.cloudera.org:8080/#/c/3013/2/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: Line 148: if (resource_metrics.has_cfile_cache_miss_bytes()) { : resource_metrics_.Increment("cfile_cache_miss_bytes", resource_metrics.cfile_cache_miss_bytes()); : } > Seems it's better to iterate the fields of ResourceMetricsPB, or we have to You can iterate the fields of the protobuf using the GetReflection() feature -- see JsonWriter::Protobuf() for an example. Line 161: UpdateResourceMetrics(); I think it's better for this function to not have any side effects. Can you move the update call to the call site? http://gerrit.cloudera.org:8080/#/c/3013/2/src/kudu/client/scanner-internal.h File src/kudu/client/scanner-internal.h: Line 32: #include "resource_metrics.h" kudu/client/ (and please sort) http://gerrit.cloudera.org:8080/#/c/3013/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 1090: kudu:: can just use cfile:: (since we are already inside the kudu:: namespace) -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: zhen.zhang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: zhen.zhang <[email protected]> Gerrit-HasComments: Yes
