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 <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
