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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
