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 11: (4 comments) Thanks for doing this, mostly looks good http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/runtime/coordinator-backend-state.h@67 PS11, Line 67: host_profiles host_profile_parent? It sounds like its an array otherwise http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/service/impala-server.cc@1021 PS11, Line 1021: std::mt19937_64 rng; This usage pattern of the RNG isn't quite right - we're constructing a new one each time to generate a single number. We could make this a member of ImpalaServer protected by a SpinLock or something like that? Or put a global RNG in the ExecEnv. http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/system-state-info.cc File be/src/util/system-state-info.cc: http://gerrit.cloudera.org:8080/#/c/12069/11/be/src/util/system-state-info.cc@54 PS11, Line 54: LOG(WARNING) << "Could not open " << path << endl; I believe you can use GetStrErrMsg() to get the error 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: thrift_profile = self.impalad_test_service.get_compressed_thrift_profile( I think we have to be a little careful with this pattern since it assumes that the query won't have been aged out of the query logs. It might be ok, but it is racy and we might turn out to regret it. http://gerrit.cloudera.org:8080/11977 actually added the option to get the encoded runtime profile via HS2 - maybe we should use that interface instead of going via the webservice? -- 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: 11 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: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 23 Jan 2019 17:07:51 +0000 Gerrit-HasComments: Yes
