Philip Zeyliger 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 8: (4 comments) 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: case TUnit::BASE_POINTS: { : DCHECK_LE(value, 10000); : ss << (value / 100); : break; : } > This prints integer values, e.g. CpuUserPercentage (500.000ms): 3, 1 Add "Prints integer values" as a comment on line 139-140, and we can call it a day :) 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 << " (thrift profile has " << num << " values, showing " << > My idea was to make it obvious where to find the whole list of values, i.e. (Showing X of Y values from Thrift Profile)? 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_FIELDS { : CPU_USER, : CPU_NICE, : CPU_SYSTEM, : CPU_IDLE, : CPU_IOWAIT : }; : : typedef std::array<int64_t, NUM_CPU_FIELDS> CpuValues; > I chose an array to iterate over it when reading and summing them up. I tri I think I do prefer a struct, but I'm open to leaving it the way it is. 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 resolution of a CPU usage counter increases once they exceed 64 > Do you have a particular way of testing it in mind? I found it most conveni Roughly, a C++ unit test could do: RuntimeProfile p; ctr = p.AddChunkedTimeSeriesCounter(); for (i in 1..67) { ctr.whatever; } pretty_string = p.PrettyString() assert something about "1, 3, 5, ..." showing up in pretty_string. Please ignore my very non-C++ syntax; I didn't have the header files open. With such a test, you can explicitly set certain values into the time series counter and check that the sampling is choosing the right stuff, and it'll be fast. -- 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: 8 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-Comment-Date: Thu, 17 Jan 2019 22:39:16 +0000 Gerrit-HasComments: Yes
