impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Fri, 01 Sep 2017 17:21:28 GMT
Sailesh Mukil has posted comments on this change.

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

Patch Set 8:

File be/src/exec/

PS7, Line 171: ++num_active_scanners_;
> num_active_scanners_ is protected by lock_. We get lock_ at the top of this
My bad, I didn't see that lock.

PS7, Line 173: scanner_threads_.AddThread(move(t));
There may be another race here where if the KuduScanNode is closed before we call scanner_threads_.AddThread(),
we may not join a running thread.

It's a little hard to follow, but I'll try to explain it as best as I can.

ThreadAvailableCb() can get called in the context of a ScannerThread (from ReleaseThreadToken()
in RunScannerThread()), whereas Close() gets called from the fragment instance execution thread.

ReleaseThreadToken() (L253) calls InvokeCallbacks() which calls ThreadAvailableCb() in the
KuduScanNode. In ThreadAvailableCb(), if there's still remaining work, we'll try to spawn
a new scanner thread again.

So if Close() gets called when a scanner thread calls ReleaseThreadToken(), we'll end up calling
scanner_threads_.JoinAll() (L133) before we call scanner_threads_.AddThread(). We also don't
synchronize with lock_ in Close() if done_ == true.

If this is a race, it looks like it existed before your code change, except that now, maybe
the window for failure is slightly larger.
File be/src/runtime/

PS8, Line 63: qs->ReleaseInitialReservationRefcount();
nit: Add a comment saying that it was taken in Init().

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0
Gerrit-PatchSet: 8
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