Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8375 )
Change subject: KUDU-2162 Expose stats about scan filters ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/8375/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8375/1//COMMIT_MSG@11 PS1, Line 11: - elapsed time processing the scan request, in nanoseconds we should also expose a client-calculated metric which is the amount of time spent waiting for responses from the server -- that should give us a good sense of whether there were delays in the network or RPC stack. (I wonder if we could expose the RPC's queue time as a trace metric as well, so we could even get that level of detail) http://gerrit.cloudera.org:8080/#/c/8375/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java File java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java: http://gerrit.cloudera.org:8080/#/c/8375/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@34 PS1, Line 34: ResourceMetricsPB if this is a public API we should not include the implementation details. http://gerrit.cloudera.org:8080/#/c/8375/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@38 PS1, Line 38: public class ResourceMetrics { needs interface annotation http://gerrit.cloudera.org:8080/#/c/8375/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@60 PS1, Line 60: return metrics.containsKey(name) ? metrics.get(name) : 0L; nit: can you use get() and then check the result vs null to avoid the double lookup? http://gerrit.cloudera.org:8080/#/c/8375/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@63 PS1, Line 63: // Updates this instance's metrics with those found in 'resourceMetricsPb'. javadoc style also worth specifying that this *adds* not replaces http://gerrit.cloudera.org:8080/#/c/8375/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@77 PS1, Line 77: Long oldAmount = metrics.containsKey(name) ? metrics.get(name) : 0L; same http://gerrit.cloudera.org:8080/#/c/8375/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8375/1/src/kudu/tserver/tablet_service.cc@1246 PS1, Line 1246: metrics->set_elapsed_time_nanos(sw.elapsed().wall); I think we should also include CPU time, because that can help determine if we're IO bound or not http://gerrit.cloudera.org:8080/#/c/8375/1/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/8375/1/src/kudu/tserver/tserver.proto@334 PS1, Line 334: optional int64 bytes_read = 4; doc this to be more specific on whether this includes data that was filtered out, etc -- To view, visit http://gerrit.cloudera.org:8080/8375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id30a7e82357fe2fc28f6d316378a612af43d8c96 Gerrit-Change-Number: 8375 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 25 Oct 2017 00:08:14 +0000 Gerrit-HasComments: Yes
