Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: PS6, Line 363: pool->ReleaseThreadToken(false, true); Do you need to drop the lock before calling this? Based on the comment from L405. Or is that why you added the 'in_callback' parameter? http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: PS6, Line 59: RETURN_IF_ERROR(Thread::Create("query-exec-mgr", : Substitute("start-query-finstances-$0", PrintId(query_id)), : &QueryExecMgr::StartQueryHelper, this, qs, &t, true)); Shouldn't we call ReleaseQueryState(qs); if the thread failed to start here? http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS6, Line 199: // If this fragment instance is not prepared, it must be reporting an error. : DCHECK(fis->IsPrepared() || !status.ok()); Is this necessarily true? We start the report thread in FragmentInstanceState::Prepare(), and we set the prepared promise after that. What if the report thread asynchronously attempts to send a status report before we set the prepared promise? http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: PS6, Line 598: Thread::Create("query-exec-state", "wait-thread", : &ClientRequestState::Wait, this, &wait_thread_); Why not inject the thread failures here as well? I think it's an important test site since the thread failure will be injected after the query starts executing. i.e. WaitAsync() fails after a call to Execute(). -- To view, visit http://gerrit.cloudera.org:8080/7730 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
