Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8375 )
Change subject: KUDU-2162 Expose stats about scan filters ...................................................................... Patch Set 8: (14 comments) C++ code looks good, just a few style nits left. http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@311 PS8, Line 311: this.resourceMetrics = new ResourceMetrics(); Can do this inline on L225 and then declare resourceMetrics final. http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@464 PS8, Line 464: public ResourceMetrics getResourceMetrics() { : return this.resourceMetrics; Got some trailing whitespace here. http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java: http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java@160 PS8, Line 160: public ResourceMetrics getResourceMetrics() { return asyncScanner.getResourceMetrics(); } Nit: not really a common Java style, especially not in this file. Could you break this up into multiple lines? http://gerrit.cloudera.org:8080/#/c/8375/8/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/8/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@42 PS8, Line 42: private HashMap<String, Long> metrics; : : public ResourceMetrics() { : metrics = new HashMap<>(); : } Could remove the constructor and declare metrics like this: private HashMap<String, Long> metrics = new HashMap<>(); Also, use implementations on the RHS, but unless you need a HashMap-specific method, store the LHS in a simpler Map. http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@60 PS8, Line 60: * @return the value of the named metric; if the metric is not found, returns 0 Hmm, but isn't it legitimate for metric values to be 0? How about we throw an exception if a metric isn't found? Or return null (and change the return type into a boxed long)? http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@67 PS8, Line 67: // Increment this instance's metric values with those found in 'resourceMetricsPb'. Convert to Javadoc. http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@69 PS8, Line 69: if (resourceMetricsPb == null) { : return; : } Seems odd to call this function with a null protobuf message. How about we precondition that it's not null and force callers to check instead? http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@72 PS8, Line 72: for (Map.Entry<FieldDescriptor, Object> entry : resourceMetricsPb.getAllFields().entrySet()) { : FieldDescriptor field = entry.getKey(); : if (field.getJavaType() == JavaType.LONG) { : increment(field.getName(), (Long) entry.getValue()); : } : } Do we really want to use protobuf reflection here? Seems like the list of fields in ResourceMetrics is short enough and fairly static, so maybe it's OK for clients to parse them one by one explicitly? Likewise, perhaps we should eschew map storage altogether and store the long fields directly? It'll be more user-friendly in that we can doc each getter and its meaning individually rather than forcing clients to discover field names at runtime (or use magic strings). http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@80 PS8, Line 80: private void increment(String name, long amount) { Doc? http://gerrit.cloudera.org:8080/#/c/8375/8/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@81 PS8, Line 81: Long oldMetricValue = metrics.get(name); : oldMetricValue = oldMetricValue != null ? oldMetricValue : 0L; : metrics.put(name, oldMetricValue + amount); Could simplify with https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#getOrDefault-java.lang.Object-V- long newAmount = metrics.getOrDefault(name, 0) + amount; metrics.put(name, newAmount); http://gerrit.cloudera.org:8080/#/c/8375/8/src/kudu/tserver/scanners.h File src/kudu/tserver/scanners.h: http://gerrit.cloudera.org:8080/#/c/8375/8/src/kudu/tserver/scanners.h@444 PS8, Line 444: explicit ScopedAddScannerTiming(Scanner* scanner, CpuTimes* cpu_times) I should have pointed this out earlier but forgot: could you update the constructor doc here to describe the effect on cpu_times? http://gerrit.cloudera.org:8080/#/c/8375/8/src/kudu/tserver/scanners.h@447 PS8, Line 447: { Nit: should be a space between ) and { http://gerrit.cloudera.org:8080/#/c/8375/8/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8375/8/src/kudu/tserver/tablet_service.cc@699 PS8, Line 699: CpuTimes* get_cpu_times() { Nit: for simple accessors like this you can just call it "cpu_times()". See https://google.github.io/styleguide/cppguide.html#Function_Names for details. http://gerrit.cloudera.org:8080/#/c/8375/8/src/kudu/tserver/tablet_service.cc@1622 PS8, Line 1622: metrics->set_cfile_cache_miss_bytes( : context->trace()->metrics()->GetMetric(cfile::CFILE_CACHE_MISS_BYTES_METRIC_NAME)); : metrics->set_cfile_cache_hit_bytes( : context->trace()->metrics()->GetMetric(cfile::CFILE_CACHE_HIT_BYTES_METRIC_NAME)); : : metrics->set_bytes_read( : context->trace()->metrics()->GetMetric(SCANNER_BYTES_READ_METRIC_NAME)); Nit: the continuation lines for these calls are indented too much. They should be indented by 4 chars. See https://google.github.io/styleguide/cppguide.html#Function_Calls for details. -- 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: 8 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 14 Nov 2019 05:57:08 +0000 Gerrit-HasComments: Yes
