Dan Hecht has posted comments on this change. Change subject: IMPALA-3857: KuduScanNode race on returning "optional" threads ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/3798/1/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: Line 427: scan_ranges_complete_counter()->Add(1); we should probably only do this at line 422. i.e. not do it if we stopped early due to done_. Line 432: DCHECK_NOTNULL(initial_range); DCHECK(initial_ranges != NULL); (we only use NONNULL if used as an expression). PS1, Line 451: If this isn't the last scanner thread, return early rather than restating what the code does, let's explain why: Don't exit if this is the last thread. Otherwise, the scan will indicate it's done before all ranges have been processed. Or something like that. PS1, Line 452: checking optional_exceeded this comment doesn't seem correct given that optional_exceeded() call isn't under the lock. I think the reason we need this code here is so that we can enter the next iteration of the loop if we find that we're the last scanner thread, and the lock is needed to make the check and decrement atomic so two threads don't give up the last option token, right? Line 477: done_ = true; I still don't see why we do this. Oh, I guess because we use it again in Close() to see if the row-batch has been shutdown yet. this seems kinda broken. Seems like Close() can just unconditionally set done_ to true and then join-all, and the last thread will do the row batch shutdown here. Anyway, no need to fix that now, you can leave as-is. http://gerrit.cloudera.org:8080/#/c/3798/1/be/src/exec/kudu-scan-node.h File be/src/exec/kudu-scan-node.h: Line 171: Status ProcessRange(KuduScanner* scanner, const TKuduKeyRange* key_range); I think this deserves it's own comment, and you could move some of the content from ScannerThread() comment to here as appropriate. -- To view, visit http://gerrit.cloudera.org:8080/3798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I22adf2109b43b1b37d9a597de85e063431dff155 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
