Dan Hecht has posted comments on this change. Change subject: IMPALA-4504: fix races in PlanFragmentExecutor regarding status reporting ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/5250/3/be/src/runtime/plan-fragment-executor.cc File be/src/runtime/plan-fragment-executor.cc: PS3, Line 295: FragmentComplete(open_status_); > Note that Close() always gets called. Do you think it would be simpler to m I thought about that but purposely wanted to avoid making RPCs in Close(), because when Sailesh was fixing those datastream sender failure cases, we decided to avoid the pattern of putting RPCs in Close because there is nowhere to bubble status up to. i.e. Close() would be about only local teardown, not interprocess teardown which added FlushFinal() for). Ultimately, I think we should consider moving all of this logic into FES (or it's predecessor) so that (a) it all happens in one place and (b) to eliminate the need for the callback indirection. PS3, Line 315: report_thread_started_cv_ > Not your bug, but this is prone to spurious wakeups. Should be: Done PS3, Line 330: RETURN_IF_ERROR > But also, now I think of it - FragmentExecState::Exec() checks to see if Pr Yeah, that's what I meant in the commit message about cleaning up the interactions. I'll go one step further to see if it seems better. -- To view, visit http://gerrit.cloudera.org:8080/5250 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I052b7b4fabb341ad27ad294cd5b0a53728d87d0e Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-HasComments: Yes
