Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: 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. http://gerrit.cloudera.org:8080/#/c/7730/8/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: PS8, Line 63: qs->ReleaseInitialReservationRefcount(); nit: Add a comment saying that it was taken in Init(). -- 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
