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 2: (10 comments) Hi Tim, thanks for the advice. I recently updated my Github Username and that lead the Gerrit not able to recognize my email and I dont find a way to correct that... So I have to create a new PR using another email. https://gerrit.cloudera.org/#/c/14837/ The code is here... Sorry for the inconvenience. http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/hbase-scan-node.cc File be/src/exec/hbase-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/hbase-scan-node.cc@39 PS2, Line 39: PROFILE_DEFINE_TIMER(TotalRawHBaseReadTime, DEBUG, > I think this could be a stable counter (I can see either low or high level) Done http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.h File be/src/exec/scan-node.h: http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.h@35 PS2, Line 35: /// Includes ScanNode common counters: > I think it's probably useful to keep these comments around for the time bei Done. I am adding all the comments back. http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.cc@56 PS2, Line 56: STABLE_LOW > I think this should be STABLE_HIGH, I think the number of bytes read is gen Done http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.cc@72 PS2, Line 72: PROFILE_DEFINE_COUNTER(NumScannerThreadsStarted, STABLE_LOW, TUnit::UNIT, > I would mark this as UNSTABLE or DEBUG - it's a pretty useless counter in p Done http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.cc@92 PS2, Line 92: DEBUG > STABLE_LOW. This and PeakScannerThreadConcurrency are pretty useful for und Done http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.cc@95 PS2, Line 95: DEBUG > STABLE_LOW Done http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/runtime/coordinator-backend-state.cc@61 PS2, Line 61: // TODO: the counters are inside the impala namespace, so we need to declare them there, > Need to remove TODO before merging. I think it makes sense just to switch f Sure, I will remove it. I was just thinking leave it so that the following up tasks can be done in the same way. http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/runtime/coordinator.cc@80 PS2, Line 80: STABLE_HIGH > Probably STABLE_LOW, this is a little tricky to interpret Done http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/util/default-path-handlers.cc File be/src/util/default-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/util/default-path-handlers.cc@130 PS2, Line 130: // name, description, unit > Comment needs updating? Done http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/util/runtime-profile.cc@68 PS2, Line 68: boost::lock_guard<SpinLock> l(lock_); > nit: don't need to use boost:: and std:: prefixes for lock_guard and vector Done -- 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: 2 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: Jiawei Wang <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 04 Dec 2019 22:22:47 +0000 Gerrit-HasComments: Yes
