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-ASF-CR] IMPALA-3857: KuduScanNode race on returning "optional" threads
Date Fri, 29 Jul 2016 01:00:58 GMT
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 <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message