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 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/11615/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11615/3//COMMIT_MSG@13 PS3, Line 13: backend states as each fragment instance tries to them at the same nit:send 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 is called at only one site and now does only one thing, should we just get rid of this method ? http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-exec-mgr.h File be/src/runtime/query-exec-mgr.h: http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-exec-mgr.h@78 PS3, Line 78: some instances hit : /// some errors nit: unless an instance hits an error they are cancelled 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) { previous patch had a racy check on the status, was there any benefit that earlier? If this also called by every fragment when they are cancelled, then it might be worth retaining the racy call http://gerrit.cloudera.org:8080/#/c/11615/3/be/src/runtime/query-state.cc@509 PS3, Line 509: } nit: retain this comment here from previously at L506: // Block until all the already started fragment instances finish Prepare()-ing to // to transition to the next state and finally report an error. -- 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: 4 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sun, 14 Oct 2018 03:22:30 +0000 Gerrit-HasComments: Yes
