impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Mon, 21 Aug 2017 20:20:00 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5750: Catch exceptions from boost thread creation

Patch Set 2:

Commit Message:

Line 34: 1. QueryState::StartFInstances() needs appropriate
> I did that for, but I think that is probably the most common
Yeah makes sense.
Commit Message:

Line 28: as arguments to the Create method, this eliminates
For testing, it might be worth adding a stress startup option to make thread creation randomly
fail. It seems hard to make a non-flaky automated test but it would be interesting to run
some queries against it and see what the behaviour is.
File be/src/exec/

Line 361:       COUNTER_ADD(num_scanner_threads_started_counter_, -1);
It seems ok to omit fixing up num_scanner_threads_started_counter_. I don't think it makes
a difference whether it counts the number of successful starts versus number of attempted
starts. Although this way is fine too.
File be/src/exec/

Line 154:     ++num_active_scanners_;
This is protected by lock_ so we could just wait until the thread is created successfully
to update it

Line 170:       COUNTER_ADD(num_scanner_threads_started_counter_, -1);
Same as the HDFS scan node - it seems ok to omit this fixup.
File be/src/rpc/

Line 37:   if (!status.ok()) throw atc::SystemResourceException("Thread::Create() failed");
This could use some explanation, since it's an unusual idiom in the code. I.e. why do we need
to throw an exception and how do we expect thrift to handle it.
File be/src/runtime/

Line 359:   Status prepare_status;
It seems like there's a fair bit of commonality between the case when Prepare() fails and
the case when thread creating fails. Maybe we can share more of the code.
File be/src/service/

Line 469:       HS2_RETURN_ERROR(return_val, status.GetDetail(), SQLSTATE_GENERAL_ERROR);
It looks like we have identical error-handling code in three places. May be cleaner to combine
into an error handling block at the end of the function + gotos.

PS3, Line 476: (void) 
File be/src/util/thread-pool.h:

Line 61:     Shutdown();
How do things get cleaned up on an error? Does Shutdown() handle that case?
File be/src/util/

Line 301:   } catch (boost::thread_resource_error& e) {
Might be worth adding a status code to common/thrift/ for this error
File be/src/util/thread.h:

PS3, Line 69: 
we'd normally use pointers for output arguments -

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message