Adar Dembo has posted comments on this change. Change subject: [c++-client] implement scan token API ......................................................................
Patch Set 6: (17 comments) http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 1187: return data_->IntoKuduScanner(scanner); Any particular reason these functions are implemented as pass-through to the Data class? I think implementing them here but forwarding the data members themselves into Data adheres to the PIMPL idiom, though maybe there's a catch I'm forgetting. http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/client.h File src/kudu/client/client.h: Line 1086: class KUDU_EXPORT ScanToken { Prefix public API classes with 'Kudu'. Same for ScanTokenBuilder. Line 1091: // Returns a KuduScanner configured for the scan token. Nit: add that the caller owns 'scanner', and that it is unset if !Status.ok(). Line 1104: // Deserialize a serialized ScanToken into a KuduScanner. Nit: who owns 'scanner'? Under what conditions? Line 1204: // Returns the schema of the projection being scanned. : KuduSchema GetProjectionSchema() const; Why does a builder need a getter method? http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_configuration.cc File src/kudu/client/scan_configuration.cc: Line 26: #include "kudu/client/scan_predicate.h" Nit: why the reordering? Line 27: #include "kudu/gutil/casts.h" What's this for? http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: Line 51: ElementDeleter deleter(&tablet_servers_); Could also adapt tablet_servers to a vector of unique_ptr during construction (or before the constructor call). Line 79: if (!ScanTokenPB::Feature_IsValid(feature) || feature == ScanTokenPB::Unknown) { Like in the Java client change: what's the point of checking against Unknown? Line 89: unique_ptr<KuduScanner> builder(new KuduScanner(table.get())); Nit: 'builder' is an odd name choice given that this is a real scanner, not a builder object. Line 160: RETURN_NOT_OK(builder->Open()); Is it weird to do this on behalf of callers? It can fail in a variety of ways and block for a while. Line 175: PartitionPruner pruner; : pruner.Init(*table->schema().schema_, table->partition_schema(), configuration_.spec()); Nit: move this down to the pruning loop? http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_token-internal.h File src/kudu/client/scan_token-internal.h: Line 25: #include "kudu/client/client.pb.h" Nit: ordering http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: Line 87: for (ScanToken* token : tokens) { Unlike the Java client test, here you're not scanning in multiple threads. Why? Line 134: // Create Session Nit: session http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/common/wire_protocol.h File src/kudu/common/wire_protocol.h: Line 103: // Convert the column predicate to protobuf. This is just code motion, right? http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 1264: boost::optional<ColumnPredicate> predicate; Is this just code deduplication? Or is something else going on that I'm not seeing? -- To view, visit http://gerrit.cloudera.org:8080/2757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If28e653a9b28164741a3c6f36097f0b7c72e8162 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
