You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2016/07/27 21:42:30 UTC

[Impala-ASF-CR] IMPALA-3857: KuduScanNode race on returning "optional" threads

Matthew Jacobs has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/3798

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

IMPALA-3857: KuduScanNode race on returning "optional" threads

The KuduScanNode could return all active scanner threads
when there were no more "optional" tokens available. In this
case, any remaining scan ranges wouldn't be picked up and
the query would produce incorrect results. This fixes the
issue by cleaning up the ScannerThread code and making sure
not to exit the last thread.

This was tested by running the tpch workload repeatedly
under load. That work to incorporate tpch data loading for
Kudu is actively being worked on, so this was tested manually.

Change-Id: I22adf2109b43b1b37d9a597de85e063431dff155
---
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
2 files changed, 64 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/3798/1
-- 
To view, visit http://gerrit.cloudera.org:8080/3798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I22adf2109b43b1b37d9a597de85e063431dff155
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-3857: KuduScanNode race on returning "optional" threads

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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 <dh...@cloudera.com>
Gerrit-HasComments: Yes