Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12116 )
Change subject: WIP IMPALA-7550 Add documentation to profile counters ...................................................................... Patch Set 4: (10 comments) Thanks for the feedback. Please see PS4 and a handful of open questions that are marked as TODO. 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: ScalarExpr::Create(target.target_expr, *row_desc(), state, &filter_expr)); > It seems like we should have different names. A standard convention would b I'll keep an eye on how often we do this and will try to find a more standardized way if it makes sense. 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@68 PS2, Line 68: PROFILE_DEFINE_COUNTER(NumBackends, STABLE, TUnit::UNIT, "Number of backends running " > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/runtime/coordinator.cc@71 PS2, Line 71: query"); > It almost seems like this should be stable. I guess there's some limitation Done 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@94 PS2, Line 94: ::impala::CounterPrototype PROFILE_##name( \ > line too long (107 > 90) Done http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@96 PS2, Line 96: > line too long (111 > 90) Done http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@107 PS2, Line 107: #nam > We usually have enum values as all caps - any reason for the difference? Done http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@132 PS2, Line 132: /// have a unique name. Subclasses then must provide a way to create new profile entries > Memory lifetime is non-obvious - document the requirement that the string m Done http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@144 PS2, Line 144: UNSTABLE, > If this approach works out simpler, I think we could make the case that it' Done http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@149 PS2, Line 149: > I guess this allows duplicate counters for now? In theory we should get linker errors but some of our counters are defined within the impala namespace while others aren't. Even if we move them all into the namespace, we cannot enforce uniqueness in the future if we declare on globally by mistake. I added some code to guarantee uniqueness during startup. http://gerrit.cloudera.org:8080/#/c/12116/2/be/src/util/runtime-profile-counters.h@160 PS2, Line 160: // CounterType, i.e. what kind of thrift counter this is > Could skip this - I feel like if we get to the point that this is a bottlen Done -- 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: 4 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 15 Jan 2019 20:35:49 +0000 Gerrit-HasComments: Yes
