David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1259: new scanner API with an encapsulated Batch object ......................................................................
Patch Set 8: (25 comments) http://gerrit.cloudera.org:8080/#/c/1562/8//COMMIT_MSG Commit Message: Line 22: quite yet remove Line 22: soversion missing space Line 37: though don't need the "though" right? http://gerrit.cloudera.org:8080/#/c/1562/8/src/kudu/client/client.h File src/kudu/client/client.h: Line 980: A single KuduScanBatch instance may be reused for better performance not sure about this. how much of a performance diff is this really going to be? moreover with the batch tied to the lifetime of the rpc controller and whatnot seems like it would make sense to just get a new KuduScanBatch each time and, otherwise users are again left to deal with semi obscure semantics. Line 981: KuduRowResult didn't you change this to something else? Line 982: derived from the batch how about: "previously obtained from batch" http://gerrit.cloudera.org:8080/#/c/1562/8/src/kudu/client/scan_batch.cc File src/kudu/client/scan_batch.cc: Line 33: extra space Line 40: } can be one line (right? i forget the minutia of the style guide here) Line 43: "~scanbatch" remove (or add meaningful info) Line 70: "cell" clarify what you mean by "cell" maybe point to another one http://gerrit.cloudera.org:8080/#/c/1562/8/src/kudu/client/scan_batch.h File src/kudu/client/scan_batch.h: Line 64: NumRows docs Line 95: weird wrapping Line 110: weird wrapping Line 124: if necessary explain what you mean by "necessary" Line 130: Should be avoided unless absolutely necessary why? Line 156: docs since this is a public header http://gerrit.cloudera.org:8080/#/c/1562/8/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: Line 424: } same q as before... can we avoid the extra line? Line 426: KuduScanBatch::Data::~Data() { same Line 442: addresses into absolute one this is a bit cryptic. How about: First, rewrite the relative offsets into absolute memory addresses. Line 465: extra lines Line 470: KuduRowResult why still use KuduRowResult here? Line 476: Extracted 0 rows more info or remove (or increase VLOGness) http://gerrit.cloudera.org:8080/#/c/1562/8/src/kudu/client/scanner-internal.h File src/kudu/client/scanner-internal.h: Line 171: extra lines http://gerrit.cloudera.org:8080/#/c/1562/8/src/kudu/rpc/rpc_controller.cc File src/kudu/rpc/rpc_controller.cc: Line 35: Canot typo http://gerrit.cloudera.org:8080/#/c/1562/8/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: Line 49: Swap docs -- To view, visit http://gerrit.cloudera.org:8080/1562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I29fd4fbb8b906ffa591853ab625ac4b089da4bc9 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Binglin Chang <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Martin Grund <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
