Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13345 )
Change subject: DWX-124: Prometheus metrics support in Impala ...................................................................... Patch Set 1: (17 comments) Did a pass over it. Mainly style and best practice comments at this stage. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h File be/src/util/collection-metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@170 PS1, Line 170: string name, std::stringstream* val, std::stringstream* metric_kind) { It's annoying that the original author of this file didn't move the ToJson(), etc methods to a .cc file. Putting non-perf-critical stuff in headers mainly blows up compile times. Anyway, you don't need to fix this, I just wanted to flag the bad practice. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@170 PS1, Line 170: string Use std::string in header files (we have a policy not to import std::string into the global namespace in headers, but some other header is probably violating that) http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@172 PS1, Line 172: string collect_data; We should just append to val instead of adding an intermediate string. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@175 PS1, Line 175: tatic_cast<uint64_t>( Why are the casts needed? this seems suspicious to me - like it might not work correctly for some types. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@178 PS1, Line 178: collect_data += name + "_last " + std::to_string(value_) + "\n"; For string concatenation we either prefer using << to append to a stringstream, or strings::Substitute() to fill out a template. Since we are already building a stringstream, we should go with the first approach. That may also avoid the need for some of this to_string() calls. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@200 PS1, Line 200: sqrt std::sqrt http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/collection-metrics.h@204 PS1, Line 204: *metric_kind << "# TYPE " + name + " counter"; use << to append to the stream instead of string concatenation. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h@81 PS1, Line 81: histogram_data += Same comments about using << to append directly to value http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/histogram-metric.h@99 PS1, Line 99: *metric_kind << "# TYPE " + name + " histogram"; Same comment about << http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h@171 PS1, Line 171: string std::string in headers http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.h@173 PS1, Line 173: *metric_kind << "# TYPE " + name + " " + PrintThriftEnum(kind()).c_str(); Use << instead of + http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc File be/src/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@190 PS1, Line 190: return; return not needed http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@206 PS1, Line 206: Don't need to add vertical whitespace http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@220 PS1, Line 220: void MetricGroup::ToPrometheus(bool include_children, std::stringstream* out_val) { Same comments apply about using << instead of strings. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@220 PS1, Line 220: std:: shouldn't need to use std:: prefix in .cc files. http://gerrit.cloudera.org:8080/#/c/13345/1/be/src/util/metrics.cc@245 PS1, Line 245: unnecessary vertical whitespace and return http://gerrit.cloudera.org:8080/#/c/13345/1/bin/distcc/distcc_server_setup.sh File bin/distcc/distcc_server_setup.sh: http://gerrit.cloudera.org:8080/#/c/13345/1/bin/distcc/distcc_server_setup.sh@58 PS1, Line 58: if ! [[ $LSB_VERSION == 14.04 || $LSB_VERSION == 16.04 || $LSB_VERSION == 18.04 ]]; then Great if this works - but can you do this in a separate change? I also wanted to know how you tested it. -- To view, visit http://gerrit.cloudera.org:8080/13345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5349085a2007b568cb97f9b8130804ea64d7bb08 Gerrit-Change-Number: 13345 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward <hars...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 15 May 2019 20:49:52 +0000 Gerrit-HasComments: Yes
