Dan Hecht has posted comments on this change. Change subject: IMPALA-5750: Catch exceptions from boost thread creation ......................................................................
Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: 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 until success to increment it so there's no window where we've started a scannerthread before the counter is incremented? is that important? http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS6, Line 199: // If this fragment instance is not prepared, it must be reporting an error. : DCHECK(fis->IsPrepared() || !status.ok()); > Is this necessarily true? I don't really understand this DCHECK. oh, because we pass 'fis' in the new case. why do we do that? the error wasn't on the instance itself, it was in the creation of the instance state, so I think we should just pass nullptr for 'fis'. the old dcheck might be racy still, as Sailesh points out. PS6, Line 328: prepare_status why is this called "prepare_status"? the old 'prepare_status' was the status of fis->Prepare(), but this is not. i think we should have a different Status for this (and it need not be outside the loop scope). 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 consistent with undoing the refcnts, or some other reason? PS6, Line 341: fis I think this should be nullptr, since the error isn't on the instance itself. http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/runtime/thread-resource-mgr.h File be/src/runtime/thread-resource-mgr.h: PS6, Line 115: bool in_callback = false); this seems complicated. why is it needed now? http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/service/child-query.h 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?) http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: PS6, Line 71: Status status; : status = request_state->WaitAsync(); nit: combine http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/util/thread.cc File be/src/util/thread.cc: Line 304: if ((nanos % 100) == 1) { on some platforms, the monotonic clock may not actually have nanosecond resolution, and so this may not work as expected. I think it would be safer to use a random number generator. Line 327: thread->swap(t); *thread = move(t) seems clearer -- 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: 6 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
