[Impala-ASF-CR] IMPALA-7550: Add documentation to profile counters

["Tim Armstrong (Code Review)" <ge...@cloudera.org>]

Tim Armstrong 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)

I think I'm getting closer to a +1. I'd want someone else to take a look and validate it too, cause this is creating the foundation for a bunch more work.

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) - we're always going to want to know how long it took to read from the storage system.


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 being - the documentation about which ones appear in single and multi-threaded scan nodes is useful.


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 generally a useful fact.


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 practice - you really care about the concurrency of the scanner threads, not how many times they started or stopped.


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 understanding how much parallelism a multithreaded scan runs with (although the counters are still hard to interpret)


http://gerrit.cloudera.org:8080/#/c/14776/2/be/src/exec/scan-node.cc@95
PS2, Line 95: DEBUG
STABLE_LOW


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 files to using namespace impala {} incrementally as counters get added to them. It's not too invasive a change.


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


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?


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 in .cc file (common/names.h imports commonly-used classes into the namespace).



-- 
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 <ji...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jiawei Wang <ji...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 19:20:19 +0000
Gerrit-HasComments: Yes