impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Mon, 28 Aug 2017 16:48:54 GMT
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 <joemcdonnell@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