Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12069 )
Change subject: IMPALA-7694: Add host resource usage metrics to profile ...................................................................... Patch Set 10: (5 comments) Thanks for the reviews. Please see PS10. http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/pretty-printer.h File be/src/util/pretty-printer.h: http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/pretty-printer.h@139 PS8, Line 139: // Printed as integer values : case TUnit::BASIS_POINTS: { : DCHECK_LE(value, 10000); : ss << (value / 100); : > Add "Prints integer values" as a comment on line 139-140, and we can call i Done http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/runtime-profile.cc@793 PS8, Line 793: stream << endl; > (Showing X of Y values from Thrift Profile)? Done http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/system-state-info.h File be/src/util/system-state-info.h: http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/system-state-info.h@70 PS8, Line 70: enum PROC_STAT_CPU_VALUES { : CPU_USER, : CPU_NICE, : CPU_SYSTEM, : CPU_IDLE, : CPU_IOWAIT : }; : : /// We store the CPU usage values in an array so that > I think I do prefer a struct, but I'm open to leaving it the way it is. I checked with Tim and we prefer to leave it the way it is for now. http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/system-state-info.cc File be/src/util/system-state-info.cc: http://gerrit.cloudera.org:8080/#/c/12069/8/be/src/util/system-state-info.cc@33 PS8, Line 33: SystemStateInfo::SystemStateInfo() { > Indeed, TIL. It looks cleaner to me, although I had to search the internet I saw that this now angers clang-tidy: /home/ubuntu/Impala/be/src/util/system-state-info.cc:33:52: warning: missing field 'system' initializer [clang-diagnostic-missing-field-initializers] I'll wait for you thoughts on array vs struct in general, but if we stay with an array I'd rather use memset than add #pragma lines to ignore this. Alternatively we could disable the warning in clang-tidy based on this discussion that points out that newer versions of GCC don't warn about this anymore. http://gerrit.cloudera.org:8080/#/c/12069/8/tests/query_test/test_observability.py File tests/query_test/test_observability.py: http://gerrit.cloudera.org:8080/#/c/12069/8/tests/query_test/test_observability.py@419 PS8, Line 419: """Tests that the thrift profile contains a time series counter for CPU resource > Roughly, a C++ unit test could do: Thanks for the pointer, I added such a test. It turned out a bit more complicated than that, but I also found a bug. :) -- To view, visit http://gerrit.cloudera.org:8080/12069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3aedc20c553ab8d7ed50f72a1a936eba151487d9 Gerrit-Change-Number: 12069 Gerrit-PatchSet: 10 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 18 Jan 2019 20:10:16 +0000 Gerrit-HasComments: Yes
