Michael Ho 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/8/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@149 PS8, Line 149: single thread Seems clearer to also state which thread it is. http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@215 PS8, Line 215: /// during Execute(). Update 'query_status_' and 'failed_instance_id_' if it's not set already. Also notify all waiters of 'instances_finished_barrier_'. http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@260 PS8, Line 260: }; nit: blank line missing. http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@261 PS8, Line 261: // nit: /// http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@268 PS8, Line 268: single QueryState thread ? http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@328 PS8, Line 328: we recieve the finst ID and the status of the : /// FIS that hit the error. the error status and failing fragment instance ID are set in 'query_status_' and 'failed_instance_id_' respectively. 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@421 PS8, Line 421: TUniqueId()) Can we also record the fragment instance id above which failed to start thread ? http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.cc@422 PS8, Line 422: Status updated_query_status = UpdateBackendExecState(); : instances_prepared_barrier_->NotifyRemaining(); Actually, there may be some problem calling NotifyRemaining() here. The problem is that some fragment instances may not have finished preparing yet at this point. In which case, if a Cancel() sneaks in after this point and before all fragment instances finished preparing, Impala may crash. Looks like we need to think through how to structure this code more carefully with WaitForPrepare() below. http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.cc@425 PS8, Line 425: all_fis_status Seems clearer to use thread_create_status here like the old code. -- 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: Mon, 30 Jul 2018 18:42:36 +0000 Gerrit-HasComments: Yes
