Sailesh Mukil 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: (9 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 Done 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 Done 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 Done 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 Done http://gerrit.cloudera.org:8080/#/c/10813/13/be/src/runtime/fragment-instance-state.cc@107 PS13, Line 107: <= > == Hmm, the enums are ordered incorrectly. I think the right thing to do would just be to adjust that. The UpdateState() function also logically expects that WAITIING_FOR_PREPARE comes before WAITING_FOR_CODEGEN. I've made the change in the thrift structure. Let me know if you have any concerns. 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 curre It should be a DCHECK. Added it. 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 thr There's a race between here and L141 and L156 in the coordinator.cc file. In L146 in coordinator.cc, the QueryState::GetFInstancesState() call blocks on WaitForPrepare(), which can unblock before the 'fis_map_' can be updated with the last fragment instance, which will cause the coordinator to cancel the query. Updated the comment above. 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 fragment There actually is no issue in sending this report earlier. But I see the confusion that's derived from the special case behavior here. Michael and I had a conversation about the same in a previous patchset. I think the code becomes more readable if we go with your suggestion. So I've added the WaitForePrepare() since that was the old behavior anyway. http://gerrit.cloudera.org:8080/#/c/10813/12/tests/failure/test_failpoints.py File tests/failure/test_failpoints.py: http://gerrit.cloudera.org:8080/#/c/10813/12/tests/failure/test_failpoints.py@162 PS12, Line 162: i = 0 > It should work with for i in range(50): Done -- 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: Sun, 05 Aug 2018 03:55:26 +0000 Gerrit-HasComments: Yes
