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 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/fragment-instance-state.cc@125 PS7, Line 125: DCHECK_ > Does DCHECK_EQ() not work ? Done http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.h@67 PS7, Line 67: : /// Eg: We transition from the PREPARING state to the EXECUTING state only if *all* the : /// underlying fragment instances have finished Prepare(). > This is not true for PREPARING phase, right ? Seems easier to separately de Done http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.h@70 PS7, Line 70: ansitioni > PREPARING ? Done http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.h@71 PS7, Line 71: y fragment instance hits an error or cancella > This is not necessarily true, right ? In PREPARING state, it only means at You're right. I changed the wording now. http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.h@264 PS7, Line 264: > May want to document the thread safety of this variable. I suppose it's onl Yup, done. http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.h@287 PS7, Line 287: /// Protected by 'status_lock_'. > Please consider documenting the thread safety of this variable too. Done http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.cc@230 PS7, Line 230: BackendExecState::EXECUTING : BackendExecState::FINISHED; : } > nit: one line Done http://gerrit.cloudera.org:8080/#/c/10813/7/be/src/runtime/query-state.cc@479 PS7, Line 479: { : unique_lock<SpinLock> l(status_lock_); : query_status_ = Status::CANCELLED; : } > Should this happen after line 483 below ? No, it makes sense to preemptively set this since a Cancel() is a query wide operation and not specific to a fragment instance. The ErrorDuring*() functions don't update the 'query_status_' if it's not OK, so that's not an issue. http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.cc@390 PS8, Line 390: // Fragment instance successfully started : fis_map_.emplace(fis->instance_id(), fis); I had to move this up here since there's a race with the coordinator between L141 and L156 in the coordinator.cc file -- 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: 8 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: Fri, 27 Jul 2018 21:42:52 +0000 Gerrit-HasComments: Yes
