Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11615 )
Change subject: IMPALA-4063: Merge report of query fragment instances per executor ...................................................................... Patch Set 5: (3 comments) as a followup to this jira, do we have any task that will look at how this will impact scaling up cluster sizes? http://gerrit.cloudera.org:8080/#/c/11615/5/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/11615/5/be/src/runtime/coordinator-backend-state.cc@283 PS5, Line 283: nit: can you briefly mention that every instance's instanceExecStatus in backend_exec_status.instance_exec_status() and its thrift profile tree in the 'TRuntimeProfileForest' are at the same index. A reader will not have to trace it back to QueryState::ConstructReport to make sure its true. On that note, does it make sense to add the instance id to the TRuntimeProfileTree, so that we can add a dcheck here and catch any bug that might be caused due to any future changes to that code? Or will that be an overkill? http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/fragment-instance-state.cc@444 PS3, Line 444: if (fragment_ctx_.fragment.output_sink.type != TDataSinkType::PLAN_ROOT_SINK) { : // if we haven't already release this thread token in Prepare(), release it now : ReleaseThreadToken(); : } > This still seems like a conceptual step in the fragment life cycle so it ma wfm http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-state.cc@397 PS3, Line 397: void QueryState::ErrorDuringExecute(const Status& status, const TUniqueId& finst_id) { > It's okay to skip the racy call for now until evidence suggests otherwise. wfm -- To view, visit http://gerrit.cloudera.org:8080/11615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f95e026ba05631f33f48ce32da6db39c6f421fa Gerrit-Change-Number: 11615 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 16 Oct 2018 21:34:29 +0000 Gerrit-HasComments: Yes
