impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3481: Use Kudu ScanToken API for scan ranges
Date Wed, 07 Sep 2016 18:34:54 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3481: Use Kudu ScanToken API for scan ranges
......................................................................


Patch Set 4:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/4120/4/be/src/exec/kudu-scan-node-test.cc
File be/src/exec/kudu-scan-node-test.cc:

Line 53: class KuduScanNodeTest : public testing::Test {
> we don't have unit tests for any other exec node. why is this an exception?
I can't say since I didn't add these to begin with, but while we're in the process of getting
our end-to-end tests up it doesn't hurt to have some of this coverage. I think once our functional
tests are in place this can be removed.


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

Line 247:     VLOG(1) << "Thread started: " << name;
> 1 is too chatty. also, please use the macros.
Done


Line 328:   VLOG(1) << "Thread done: " << name;
> change
Done


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

Line 39: /// are used to retreive the rows for this scan.
> retrieve
Done


Line 120:   void ScannerThread(const string& name, const string* initial_token);
> rename to some verb
Done


Line 125:   Status ProcessScanToken(KuduScanner* scanner, const string* scan_token);
> why const*? can this be null?
no, thanks, this should be const&


http://gerrit.cloudera.org:8080/#/c/4120/4/be/src/exec/kudu-scanner.cc
File be/src/exec/kudu-scanner.cc:

Line 114:       if (batch_done) break;
> why break instead of return?
To avoid returning Status::OK() again, and in case logic gets added after the while() loop.
If you think it's simpler returning OK() here I'm happy to change it.


http://gerrit.cloudera.org:8080/#/c/4120/4/be/src/exec/kudu-scanner.h
File be/src/exec/kudu-scanner.h:

Line 34: /// Wraps a Kudu client scanner to fetches row batches from Kudu. The Kudu client
scanner
> to fetch
Done


Line 114:   int cur_kudu_batch_num_scanned_;
> explain
Done


http://gerrit.cloudera.org:8080/#/c/4120/4/fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java
File fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java:

Line 107:     try (KuduClient client = new KuduClientBuilder(
> break before 'new' instead
Done


Line 165:       // TODO: Consider only using the LEADER replica.
> for better cache locality?
I was thinking it might be better but I don't know if there's any reason that might be the
case. I'll take out this comment unless and we can chat about the scheduling policies later.


Line 167:         TNetworkAddress address = new TNetworkAddress(replica.getRpcHost(),
> break before new instead
Done


Line 203:     for (KuduPredicate predicate: kuduPredicates_) {
> single line
Done


Line 267:    * bounds of the result can be evaluated with Expr::GetValue(NULL)).
> this presumable has some side effects. describe them.
Done


Line 349:   private static ComparisonOp getKuduOperator(BinaryPredicate.Operator op) {
> use qualified name for ComparisonOp, easier to identify that it's a "foreig
Done


Line 351:     case GT: return ComparisonOp.GREATER;
> indentation
Done


http://gerrit.cloudera.org:8080/#/c/4120/4/fe/src/test/java/com/cloudera/impala/planner/KuduPlannerTest.java
File fe/src/test/java/com/cloudera/impala/planner/KuduPlannerTest.java:

Line 33: public class KuduPlannerTest extends PlannerTestBase {
> could we merge this back into the regular planner test? what was the reason
I don't know what the original reason was, but now that it's separate it's kind of handy for
creating/destroying the KuduClient only for these tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/4120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I160e5849d372755748ff5ba3c90a4651c804b220
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Dan Burkert <dan@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <todd@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message