Tim Armstrong has posted comments on this change. Change subject: IMPALA-5895: clean up runtime profile lifecycle ......................................................................
Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/8069/7/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: PS7, Line 85: DCHECK(!has_active_periodic_counters_); > We shouldn't take locks only under debug builds since they have side effect Also the destructor can't safely run concurrently with other method calls regardless. PS7, Line 88: void RuntimeProfile::StopPeriodicCounters() { > This doesn't stop the counters that belong to child profiles. Is it on the Yeah, my thinking was that whatever added the counters to the profile should also explicitly stop them. Updated the method comment since it wasn't very clear. http://gerrit.cloudera.org:8080/#/c/8069/7/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: PS7, Line 407: rate_counters > nit: rate_counters_ Done PS7, Line 408: sampling_counters > nit: sampling_counters_ Done -- To view, visit http://gerrit.cloudera.org:8080/8069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
