Tim Armstrong has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/7730/3//COMMIT_MSG Commit Message: Line 28: variables or direct declarations in classes. > I have done hand testing on a variety of codepaths. I'm looking into automa Yeah, it does seem like there's an important distinction there. http://gerrit.cloudera.org:8080/#/c/7730/3/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 361: COUNTER_ADD(num_scanner_threads_started_counter_, -1); > I'm ok with either way. My thought is that in cases where the query doesn't Oh I see. Seems ok. I'm a little skeptical that it's worth trying to recover from this instead of just failing the query. http://gerrit.cloudera.org:8080/#/c/7730/4/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: Line 164: auto fn = [this, token, name]() { this->RunScannerThread(name, token); }; In this case it does seem simpler to fail the query, since it we mess this up we might drop a scan range and give incorrect results. Your change looks correct but it seems like it would be easy for someone to accidentally break it. http://gerrit.cloudera.org:8080/#/c/7730/4/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: Line 330: initial_reservation_refcnt_.Add(1); // decremented in ExecFInstance() Still relevant? Line 350: // Undo refcnts I think we should preserve the postcondition that all fragment instances have finished Prepare() when this function returns, just to reduce the possible divergence in behaviour from the normal path. http://gerrit.cloudera.org:8080/#/c/7730/3/be/src/util/thread-pool.h File be/src/util/thread-pool.h: Line 61: /// error spawning the threads. > When we get an error, only successfully started threads are in the threads_ Yeah I think that would make most sense to me - reduces the number of different things that can happen. I guess it would also be good to reduce the number of threads running sooner rather than later (although it does seem like tearing things down in the destructor would mostly be ok). http://gerrit.cloudera.org:8080/#/c/7730/3/be/src/util/thread.cc File be/src/util/thread.cc: Line 301: return Status(e.what()); > Added this. I don't think the category or name would be useful to the custo Sounds like a good idea to me. -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
