Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10158 )
Change subject: IMPALA-5384, part 2: Simplify Coordinator locking and clarify state ...................................................................... Patch Set 15: (11 comments) http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.h@341 PS15, Line 341: failed_fragment' nit: update to "failed_finstance" http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.h@414 PS15, Line 414: nit: extra space http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@121 PS14, Line 121: s not thread should we call this a non-OK status, because for cancellation through Cancel() we should not call it error http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@131 PS14, Line 131: tate_ do you mean cancels? http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc@147 PS15, Line 147: Status prepare_status = query_state_->WaitForPrepare(); : DCHECK(!prepare_status.ok()); : return prepare_status; should we call UpdateExecStateIfError() here? http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc@486 PS15, Line 486: !status.ok() && exec_status_.IsCancelled() can we have a case with <state, status> = <ERROR, Cancelled> ? http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc@522 PS15, Line 522: // TODO: is this needed? This will also happen on the "backend" side of cancel rpc. : // And in the case of EOS, sink already knows this. : if (coord_sink_ != nullptr) coord_sink_->CloseConsumer(); I dont think we need this, I looked at why we originally needed this and it was because the fragment lifecycle was a little different and we needed to to close the sink in case the Prepare phase of the fragments failed. After patch IMPALA-2550, we handle partially-prepared state failure differently and hence dont require this anymore. Moreover, since this was used to close partially-prepared state failure and now if we fail prepare, coord_sink_ is not even set, so this is essentially redundant. http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc@530 PS15, Line 530: ensure resources are : // really freed before admitting more queries? in case of cancelBackends(), should we also wait for resources to be freed? http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc@605 PS15, Line 605: UpdateExecStateIfError maybe change the name to something like UpdateExecState, since this is also used to only get the overall status and it may or may not be an error status http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc@639 PS15, Line 639: // Wait until backends have started executing, otherwise the cancel rpc might arrive : // before the exec rpc. : exec_rpcs_complete_barrier_->Wait(); does this mean we can call cancel before Exec() completes? what if cancel is called before exec_rpcs_complete_barrier_ is initialized and is still nullptr. We should probably just mark this method to only be called after Exec completes because currently that how it is used. http://gerrit.cloudera.org:8080/#/c/10158/15/be/src/runtime/coordinator.cc@943 PS15, Line 943: backend_states_init_lock_ Do we expect these methods to be called before Exec() completes? If not, we can get rid of backend_states_init_lock_ and mark these methods to only be called after Exec() -- To view, visit http://gerrit.cloudera.org:8080/10158 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1abdfd02163f9356c59d470fe1c64ebe012a9e8e Gerrit-Change-Number: 10158 Gerrit-PatchSet: 15 Gerrit-Owner: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 09 May 2018 20:55:25 +0000 Gerrit-HasComments: Yes
