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 11: (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: } : // Tell the managing 'QueryState' that we're done with executing and that we've stopped : // the reporting thread. : query_state_->DoneExecuting(); : : done: : if (!status.ok() && current_state_.Load() <= TFInstanceExecState::WAITING_FOR_PREPARE) { : // Tell the managing 'QueryS > Should this be included moved to point after label done ? Otherwise, we may Great catch. I took your suggestion and moved the ErrorDuring* calls to the 'done:' section. I also added a debug action to fail 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: ExecState o > not used ? Done http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@217 PS10, Line 217: > BackendExecState old_state = backend_exec_state_; and no need for line 220 Done http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@230 PS10, Line 230: return query_status_; : } > nit: one line Done http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@408 PS10, Line 408: ReleaseExecResourceRefcount(); : ExecEnv::GetInstance()->query_exec_mgr()->ReleaseQueryState(this); : break; : } : // update fragment_map_ : vector<FragmentInstanceState*>& fis_list = fragment_map_[instance_ctx.fragment_idx]; : fis_list.push_back(fis); : t->Detach(); : --n > A minor suggestion is to group this loop with ErrorDuringPrepare() below so Good point. Done. http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@435 PS10, Line 435: ReportExecStatusAux(true, thread_create_status, nullptr, true); > We used to wait for all fragment instances to finish preparing before repor No, if a Cancel() arrives due to this before all the fragment instances have finished preparing, Cancel() will block on WaitForPrepare(). We'll end up cancelling the query earlier which is good. http://gerrit.cloudera.org:8080/#/c/10813/10/be/src/runtime/query-state.cc@442 PS10, Line 442: BackendExecStat > not actually used? Done 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 ? execute_query_expect_failure() does that internally. http://gerrit.cloudera.org:8080/#/c/10813/10/tests/failure/test_failpoints.py@157 PS10, Line 157: query_options={'debug_action':debug_action}) : : # Fail the fragment instance thread creation with a 0.5 probability. : debug_action = 'FIS_FAIL > for i in range(50): Done http://gerrit.cloudera.org:8080/#/c/10813/10/tests/failure/test_failpoints.py@165 PS10, Line 165: while i in range(50): : try: : self.execute_query(query, : query_options={'de > assert 'Query aborted:Debug Action: FIS_FAIL_THREAD_CREATION:[email protected]' in s 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: 11 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 02:55:12 +0000 Gerrit-HasComments: Yes
