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 13: (7 comments) http://gerrit.cloudera.org:8080/#/c/10813/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10813/13//COMMIT_MSG@28 PS13, Line 28: The fragment instances update the query wide query status if an error is hit nit: a few long lines here http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.h@79 PS13, Line 79: hould WaitForPrepare nit: update comment http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc@86 PS13, Line 86: if (!status.ok()) { : goto done; : } nit: one line http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc@98 PS13, Line 98: if (!status.ok()) { : goto done; : } nit: one line http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/query-state.cc@217 PS13, Line 217: The method comment mentions that : "A state transition happens if the current state is a non-terminal state" maybe add if(IsTerminalState(old_state)) return query_status_; or add a dcheck for that otherwise we can potentially set an error/cancel state after BackendExecState::FINISHED http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/query-state.cc@390 PS13, Line 390: fis_map_.emplace(fis->instance_id(), fis); any specific reason why you moved this before you actually start up the thread? If you keep it here, then maybe update the comment above it http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/query-state.cc@435 PS13, Line 435: ReportExecStatusAux(true, thread_create_status, nullptr, true); looks like we can now return a done exec status before the started fragments complete prepare phase. Is that expected? or should we wait for prepare here. the commit message says that: "The PREPARING state blocks regardless of whether a fragment instance hit an error or not, until all the fragment instances have completed successfully or unsuccessfully" we should probably call wait for prepare before calling UpdateBackendExecState() -- 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: 13 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Sat, 04 Aug 2018 03:45:28 +0000 Gerrit-HasComments: Yes
