Dan Hecht has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: Line 226: if (fis->IsPrepared()) { should this check that the prepare status is ok? if Prepare() failed, then these fields aren't guaranteed to be set, right? alternatively, we coudl check for state != NULL ptr, like we are doing for profile() and like in FragmentInstanceState::Close(). Line 328: ExecEnv::GetInstance()->query_exec_mgr()->ReleaseQueryState(this); this is a little weird since ReleaseQueryState() can delete this. Except it won't since the the QueryExcMgr is holding a refcnt. Could you augment line 287 to make it a bit more obvious, like line 288 does: << "Taken in QueryExecMgr::StartQuery()" PS8, Line 332: true passing instances_started=true can lead to a deadlock if the RPC send fails (it's used to decide whether to call Cancel(), which waits for instances_prepared_promise_ to be set but that doesn't happen until later...) PS8, Line 336: update fis_map_ nit: seems excessive http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/runtime/thread-resource-mgr.h File be/src/runtime/thread-resource-mgr.h: Line 288: if (!in_callback) InvokeCallbacks(); > You're right. This is only safe from a callback if a thread did TryAcquireT As we discussed in person, I don't think it's safe even in that case generally, because while the callback held the token, another client may have wanted to start a thread, and now won't get a callback. I think these two options would be okay: 1) Rename 'in_callback' to 'skip_callbacks'. That way, the side-effect is more obvious to the caller, and it's a decision the caller needs to make. In the callsite you are adding, we should then document that the callbacks need to be skipped to avoid recursion, but also that it is safe to do so since the system has hit the thread limit and so there's no point in doing further callbacks right now. 2) Alternatively, you could pass back an "id" when registering the callback (say, the index of the callback), and then pass that in here so that the callback is skipped. Given that the ThreadResourceMgr has a limited lifespan (with MT work), option #1 seems okay and easier to reason about right now. -- 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: 8 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
