Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10813 )
Change subject: IMPALA-7163: Implement a state machine for the QueryState class ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.h@152 PS6, Line 152: /// exited the INITALIZED state. update comment to mention that it blocks till a terminal state has been reached (since it call waitForFinishInternal()) http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.h@156 PS6, Line 156: Same as WaitForPrepareInternal nit: avoid reference to private methods. how about switching the description and adding to it in the private method instead, like "Helper method for WaitForPrepare(), does the same but returns a FInstanceStatus instead http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.h@281 PS6, Line 281: CANCELLED when is the cancelled state set? http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.h@289 PS6, Line 289: ERROR if cancelled, will the status be Status::CANCELLED? if yes that is also counted as an error/non-ok state and BackendExecState will be set to ERROR http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.cc@233 PS6, Line 233: TUniqueId failed_instance = finst_status.finst_id_; unused variable. maybe just pass a status object to this method if the finst_id_ is not used. Also, I noticed that the finst_id_ is stored during call to ErrorDuringPrepare() and ErrorDuringExecute() but dosent seem to be used anywhere. we can probably get rid of the struct. or am I missing something? http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.cc@241 PS6, Line 241: if (!status.ok()) { : // Error while executing - go to ERROR state. : query_status_ = finst_status; : backend_exec_state_ = BackendExecState::ERROR; : } do we have a cancellation path? http://gerrit.cloudera.org:8080/#/c/10813/6/be/src/runtime/query-state.cc@379 PS6, Line 379: instances_finished_barrier_->Wait(); unique_lock<SpinLock> l(status_lock_)? -- To view, visit http://gerrit.cloudera.org:8080/10813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec5670a7db83ecae4656d7bb2ea372d3767ba7fe Gerrit-Change-Number: 10813 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Thu, 12 Jul 2018 01:45:19 +0000 Gerrit-HasComments: Yes
