Jiawei Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14776 )
Change subject: IMPALA-7550: Add documentation to profile counters ...................................................................... Patch Set 1: (6 comments) Hi Tim, Lars, other Impala team members This is the first edition of impala profile counters that I had based on Lars' previous work on https://gerrit.cloudera.org/#/c/12116/ Thanks so much for Lars building the basis. I had several comments on the commit itself. Any suggestions on how to improve it is highly welcomed! Thanks so much! http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/exec/hdfs-scan-node-base.h@525 PS1, Line 525: /// Disk accessed bitmap I am not too sure about the following two counters. Wondering if we should just leave it like it is. http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/exec/hdfs-scan-node-base.cc@127 PS1, Line 127: PROFILE_DEFINE_SUMMARY_STATS_COUNTER(ParquetUncompressedBytesReadPerColumn, STABLE, This seems redundant, any advice to modify it will be appreciated. http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/exec/hdfs-scan-node-base.cc@139 PS1, Line 139: PROFILE_DEFINE_COUNTER(DataCacheHitCount, STABLE, HIGH, TUnit::UNIT, Did not find the explanation for the following data cache counters. Might need some other better explanation. http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/exec/scan-node.cc@82 PS1, Line 82: PROFILE_DEFINE_TIMER(RowBatchQueueGetWaitTime, UNSTABLE, LOW, "Wall clock time that the " Hi, Tim I believe these are the Timer you mentioned in the threads. Any suggestions on how should we handle it other than put it in the descriptions? http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/14776/1/be/src/util/runtime-profile-counters.h@164 PS1, Line 164: enum class Significance { I add this Significance to measure if the counter is important to a user. Maybe we can also combine this with stability or redefine the stability? I am not too sure about this. http://gerrit.cloudera.org:8080/#/c/14776/1/www/profile_docs.tmpl File www/profile_docs.tmpl: http://gerrit.cloudera.org:8080/#/c/14776/1/www/profile_docs.tmpl@1 PS1, Line 1: <!-- Screenshot of this can be found here: https://drive.google.com/file/d/1_FdwFxkgfuDqKXM71jt7DQwqJ6v_LGqY/view?usp=sharing Any advice is welcomed. -- To view, visit http://gerrit.cloudera.org:8080/14776 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc03faddb27754001290bb6d899840e2cbe7ccb7 Gerrit-Change-Number: 14776 Gerrit-PatchSet: 1 Gerrit-Owner: Jiawei Wang <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jiawei Wang <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 21 Nov 2019 23:40:53 +0000 Gerrit-HasComments: Yes
