impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Ribeiro Alves (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3857: KuduScanNode race on returning "optional" threads
Date Wed, 13 Jul 2016 22:50:08 GMT
David Ribeiro Alves has posted comments on this change.

Change subject: IMPALA-3857: KuduScanNode race on returning "optional" threads
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3637/1/be/src/exec/kudu-scan-node.cc
File be/src/exec/kudu-scan-node.cc:

Line 421:   bool optional_threads_exceeded = false;
anyway to add a simple directed test to kudu-scan-node-test.cc for this?


PS1, Line 450: return
rephrase "return this thread"


Line 480:   if (!optional_threads_exceeded) --num_active_scanners_;
do we even need this bool? seems like the only case when the condition is false is when we've
decremented above, in which case isn't it the same thing to do it here?

I mean, couldn't you just have, above:

{
unique_lock<mutex> l(lock_);
      if (num_active_scanners_ > 1 &&
          runtime_state_->resource_pool()->optional_exceeded()) {
        goto done;
 }

and then not have this change at all?


-- 
To view, visit http://gerrit.cloudera.org:8080/3637
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I22adf2109b43b1b37d9a597de85e063431dff155
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dralves@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message