Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12116 )
Change subject: WIP IMPALA-7550 Add documentation to profile counters ...................................................................... Patch Set 2: (7 comments) I'm in favour of going forward with this. I guess we'd incrementally document counters and as the final step make it impossible to create the counters without registering them? http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/exec/scan-node.cc@144 PS2, Line 144: // TODO: What to do with this? We probably shouldn't have counters with the same name It seems like we should have different names. A standard convention would be nice though. What you're doing here seems fine. It looks like there's only a handful of these time series counters in the codebase to update. http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/runtime/coordinator.cc@71 PS2, Line 71: unstable It almost seems like this should be stable. I guess there's some limitations, like we don't include CPU time outside of query execution (coordination, planning, etc). http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@107 PS2, Line 107: stable We usually have enum values as all caps - any reason for the difference? http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@132 PS2, Line 132: const char* name_; Memory lifetime is non-obvious - document the requirement that the string memory has process lifetime? http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@144 PS2, Line 144: // TODO: live in ExecEnv? If this approach works out simpler, I think we could make the case that it's different from the members of ExecEnv because it's "static" data. http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@149 PS2, Line 149: void AddPrototype(const ProfileEntryPrototype* prototype) { I guess this allows duplicate counters for now? I guess we might get linker errors on duplicate counters if the symbols are exported from the compilation units, which I think they are? http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@160 PS2, Line 160: // todo: separate r/w locking Could skip this - I feel like if we get to the point that this is a bottleneck, we've got bigger issues. -- To view, visit http://gerrit.cloudera.org:8080/12116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idaa0a44f0a996f3487566b545d984d562e6e1588 Gerrit-Change-Number: 12116 Gerrit-PatchSet: 2 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: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 10 Jan 2019 01:35:02 +0000 Gerrit-HasComments: Yes
