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 9: (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. Done 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. A We usually avoid calling out private members in the header comments of public functions, so I've added and reworded your suggestion to only using public names. http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@260 PS8, Line 260: }; > nit: blank line missing. Done http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@261 PS8, Line 261: // > nit: /// Done http://gerrit.cloudera.org:8080/#/c/10813/8/be/src/runtime/query-state.h@268 PS8, Line 268: single > QueryState thread ? Done 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_ Done 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 thr Isn't this more of a QueryState failure though? Since it has nothing to do the fragment instance itself, but is rather the failure to start a thread. If you think it'll help knowing *which* fragment instance was the first to fail to start, then we could add it. But I can't think of a scenario where that might come in handy. 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 pro You're right. I missed that. I've added logic to Notify() the 'instances_prepared_barrier_' as many times as required, from this thread, if we fail to start fragment instance threads. I've also added a debug action here to ensure that this doesn't cause any crashes or hangs. 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. 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: 9 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@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: Wed, 01 Aug 2018 02:11:58 +0000 Gerrit-HasComments: Yes
