Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12069 )
Change subject: IMPALA-7694: Add host resource usage metrics to profile ...................................................................... Patch Set 13: (5 comments) My main outstanding concerns are the ones that Michael raised. Another pass revealed a few more questions. http://gerrit.cloudera.org:8080/#/c/12069/13/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/12069/13/be/src/runtime/coordinator.cc@789 PS13, Line 789: // TODO: Move to host profiles? This seems like the right thing to do. Maybe file a JIRA to track? We could also simplify BackendState::ComputeResourceUtilization() to just use the per-backend counters instead of iterating over fragments. I think there may be some compatibility concerns about removing these - existence of the counters isn't contractual but we don't want to break useful tools if avoidable. For example, I confirmed that Cloudera Manager actually does parse the existing strings (which is a little sad, but understandable given the lack of other counters). http://gerrit.cloudera.org:8080/#/c/12069/13/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/12069/13/be/src/runtime/query-state.cc@151 PS13, Line 151: CpuUserPercentage Should we add a System prefix to these or similar? Otherwise it could be interpreted as the percentage of cpu used by this query. http://gerrit.cloudera.org:8080/#/c/12069/13/be/src/runtime/query-state.cc@230 PS13, Line 230: host_profile_, query_id(), query_options().scratch_limit)); I'm actually super-happy that we get these counters now. http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/runtime-profile.h@419 PS11, Line 419: Samples : /// are not re-sampled into larger intervals, instead owners of this profile can call : /// ClearChunkedTimeSeriesCounters() to reset the sample buffers of all chunked time : /// series counters > Yes. If you think that's a problem, we can cap the number of samples at som I hadn't thought about this, but I think it's worth doing, just so that we can reason about the behaviour better. http://gerrit.cloudera.org:8080/#/c/12069/11/tests/observability/test_plot_profile_resource_usage.py File tests/observability/test_plot_profile_resource_usage.py: http://gerrit.cloudera.org:8080/#/c/12069/11/tests/observability/test_plot_profile_resource_usage.py@35 PS11, Line 35: > I agree that this should be done through Impyla, but it doesn't expose the I don't want to hold back good work, I can live with this I think. -- To view, visit http://gerrit.cloudera.org:8080/12069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3aedc20c553ab8d7ed50f72a1a936eba151487d9 Gerrit-Change-Number: 12069 Gerrit-PatchSet: 13 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Sat, 26 Jan 2019 00:48:53 +0000 Gerrit-HasComments: Yes
