impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3857: KuduScanNode race on returning "optional" threads
Date Thu, 21 Jul 2016 17:16:58 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 2:

(2 comments)

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

PS2, Line 451: num_active_scanners_ > 1
> I tried splitting lock_ up to have a separate lock for this scenario, to ma
Hmm, it seems like this is still a practical problem though.  Suppose we have 10 threads,
but our quote is now 9.  All 10 can see optional_exceeded()==true and with this fix, we'll
shutdown 9.  So we'll be able to make progress, but performance will be 9x slower than it
should be (and 9x slower than when we do not hit the race). That's going to lead to unnecessary
and difficult perf investigations.

Also, did you try the opposite factoring I suggested in the comment for L456?  That is, it
seems it would be cleaner to factor out lines 424-445 into a "ProcessRange(key_range)" routine,
and then you shouldn't need goto's and can have a single exit path.


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

PS3, Line 166: 'initial_rang
i think we do try to use single quotes to distinguish variable names.


-- 
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: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dralves@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message