Michael Ho 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: (4 comments) 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 This still seems like a conceptual step in the fragment life cycle so it makes sense to encapsulate it as a function. 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: an instances hit : /// an error or > nit: unless an instance hits an error they are cancelled Done 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 e It's okay to skip the racy call for now until evidence suggests otherwise. It's not a big deal if multiple fragment threads have to block when setting the error status. We don't expect this to be a common case so it's better to keep the code simpler and less error prone. 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: Done -- 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 <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: Tue, 16 Oct 2018 00:47:50 +0000 Gerrit-HasComments: Yes
