Joe McDonnell has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: PS7, Line 371: COUNTER_ADD(num_scanner_threads_started_counter_, 1); > The problem with moving this here is when the node is under heavy stress, t Yes, this could be off. I suppose there is no way around it. 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_; > This can cause quite a few races. It can race with L220, L244 and L245. num_active_scanners_ is protected by lock_. We get lock_ at the top of this loop and the other locations get lock_. 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: status = Thread::Create("query-exec-mgr", : Substitute("start-query-finstances-$0", PrintId(query_id)), : &QueryExecMgr::StartQueryHelper, this, qs, &t, true); > Done In testing, I found that this also needs qs->ReleaseInitialReservationRefcount(), which was taken in Init(). Added. http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS7, Line 335: // Fragment instance successfully started : // update fis_map_ : fis_map_.emplace(fis->instance_id(), fis); : // update fragment_map_ : vector<FragmentInstanceState*>& fis_list = fragment_map_[instance_ctx.fragment_idx]; : fis_list.push_back(fis); > Is it safe to update the map with the Fragment instance state after already My thought process is something like this: fis is a local variable. Both fis_map_ and fragment_map_ are private variables on QueryState. None of these variables are referenced outside this QueryState except through APIs protected by the promise. It looks like the fragment instance doesn't care about using the functions that would access these variables. Worth considering if there is any way around it, but I think it is safe. http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/util/thread.cc File be/src/util/thread.cc: PS7, Line 303: rand() > Not to be pedantic, but a rand() without seeding the PRNG first, will cause I was thinking it might make sense for us to do a single srand() at startup. I will look into the appropriate place to do this. -- 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: 7 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
