impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joe McDonnell (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Thu, 31 Aug 2017 20:22:29 GMT
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5750: Catch exceptions from boost thread creation

Patch Set 6:

File be/src/exec/

PS6, Line 159:     Status status;
             :     status =
> nit: combine these lines

PS6, Line 163: num_scanner_threads_started_counter_
> here and other scan node: I guess we undo this rather than just waiting unt
I checked, and this is only used as a statistic and for making the names of the threads unique.
I changed this to increment after successful thread create (here and for
(A side effect is that the thread indexes start from zero.)
File be/src/runtime/

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));
> Yes, that looks like a bug. I will fix in the next upload.
File be/src/runtime/

PS6, Line 199:     // If this fragment instance is not prepared, it must be reporting an error.
             :     DCHECK(fis->IsPrepared() || !status.ok());
> If fis==null case doesn't report the error, then that's a bug we need to fi
Looking into a fix. I will also do fault injection at that point to see what happens. The
two might be slightly different, because in that case no fragment instances have been started.

PS6, Line 331:       // Remove fis from fis_map_ and fragment_map_
             :       fis_map_.erase(fis->instance_id());
             :       fis_list.pop_back();
> why don't we just do this on the success path? is it to try to be consisten
I checked and all other uses of these two things check that the instances_preapred_promise_
is set. So, I moved it to the success path, since that is safe.
File be/src/runtime/thread-resource-mgr.h:

PS6, Line 115: bool in_callback = false);
> this seems complicated. why is it needed now?
ReleaseThreadToken calls InvokeCallbacks. The two locations where we use this (
and are in functions that are registered as callbacks. If we try to release
inside the callback, this is mutual recursion.
File be/src/service/child-query.h:

Line 159:   Status ExecAsync(std::vector<ChildQuery>&& child_queries);
> WARN_UNUSED_RESULT (or are we relying on the Status annotation now?)
File be/src/service/

PS6, Line 598: Thread::Create("query-exec-state", "wait-thread",
             :       &ClientRequestState::Wait, this, &wait_thread_);
> Good point. I will add it to the list of locations that are eligible for fa
File be/src/service/

PS6, Line 71:   Status status;
            :   status = request_state->WaitAsync();
> nit: combine
File be/src/util/

Line 304:     if ((nanos % 100) == 1) {
> Good point, will make that change.
Changed to use rand()

Line 327:   thread->swap(t);
> *thread = move(t) seems clearer

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message