impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Wed, 30 Aug 2017 23:49:48 GMT
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 <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message