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 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/fragment-instance-state.cc@100 PS10, Line 100: if (!status.ok()) { : // Tell the managing 'QueryState' that we hit an error during execution. : query_state_->ErrorDuringExecute(status, instance_id()); : goto done; : } : // Tell the managing 'QueryState' that we're done with executing and that we've stopped : // the reporting thread. : query_state_->DoneExecuting(); Should this be included moved to point after label done ? Otherwise, we may hang in case Open() fails ? May be this warrants a debug action which triggers failure() in Open(). http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@216 PS10, Line 216: ret_status; not used ? http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@217 PS10, Line 217: BackendExecState old_state; BackendExecState old_state = backend_exec_state_; and no need for line 220 http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@230 PS10, Line 230: BackendExecState::EXECUTING : : BackendExecState::FINISHED; nit: one line http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@408 PS10, Line 408: // We failed to start 'num_unstarted_instances', so make sure to notify : // 'instances_prepared_barrier_' 'num_unstarted_instances - 1' times, to unblock : // 'WaitForPrepare(). The last remaining notification will be set by the error : // handling logic once we've exited this loop so that the we may have the query : // wide status reflect the 'thread_create_status'. : while (num_unstarted_instances > 1) { : DonePreparing(); : --num_unstarted_instances; : } A minor suggestion is to group this loop with ErrorDuringPrepare() below so it's easier to see that we have updated num_unstarted_instances times. http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@435 PS10, Line 435: ErrorDuringPrepare(thread_create_status, TUniqueId()); We used to wait for all fragment instances to finish preparing before reporting errors. This patch stops doing that. Does it matter ? http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@442 PS10, Line 442: all_fis_status; not actually used? WaitForPrepare() below can become discard_result(WaitForPrepare()); http://gerrit.cloudera.org:8080/#/c/10813/10/tests/failure/test_failpoints.py File tests/failure/test_failpoints.py: http://gerrit.cloudera.org:8080/#/c/10813/10/tests/failure/test_failpoints.py@151 PS10, Line 151: self.execute_query_expect_failure(self.client, query, : query_options={'debug_action':debug_action}) Do we care about verifying the failure actually occurred ? http://gerrit.cloudera.org:8080/#/c/10813/10/tests/failure/test_failpoints.py@157 PS10, Line 157: x = 0 : # Since there's only a 50% chance of fragment instance thread creation failure, : # we attempt to catch a query failure up to a very conservative maximum of 50 tries. : while x in range(0, 50): for i in range(50): http://gerrit.cloudera.org:8080/#/c/10813/10/tests/failure/test_failpoints.py@165 PS10, Line 165: if 'Query aborted:Debug Action: FIS_FAIL_THREAD_CREATION:[email protected]' in str(e): : break : else: : assert False, str(e) assert 'Query aborted:Debug Action: FIS_FAIL_THREAD_CREATION:[email protected]' in str(e), str(e) -- 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: 10 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: Thu, 02 Aug 2018 00:06:19 +0000 Gerrit-HasComments: Yes
