Todd Lipcon has posted comments on this change. Change subject: KUDU-1259: new scanner API with an encapsulated Batch object ......................................................................
Patch Set 8: (22 comments) lots of the comments were on moved chunks of code rather than new code, so I didn't change those (to preserve history). If you feel like the cleanup's worth doing we can do another patch on top for those. http://gerrit.cloudera.org:8080/#/c/1562/8//COMMIT_MSG Commit Message: Line 22: quite yet > remove Done Line 22: soversion > missing space "soversion" is actually a term (https://cmake.org/cmake/help/v3.0/prop_tgt/SOVERSION.html) Line 37: though > don't need the "though" right? Done 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 It's better because it ends up re-using the same buffers for the responses, if you keep reusing the same scanbatch object. So, less allocator pressure and better cache locality. The RPC controller is entirely embedded here - the user shouldn't have to worry about it. From the user's perspective it's pretty easy - just like any other "out-param" you can reuse it, and the reuse will replace the existing one, but reuse some of its storage (like how reusing a string will avoid having to realloc) Line 981: KuduRowResult > didn't you change this to something else? Done Line 982: derived from the batch > how about: "previously obtained from batch" Done 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 Done Line 40: } > can be one line (right? i forget the minutia of the style guide here) Done Line 43: "~scanbatch" > remove (or add meaningful info) woops, accidentally left in some debug Line 70: "cell" > clarify what you mean by "cell" maybe point to another one this is just moved code, rather than new code 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 Done Line 95: > weird wrapping this and the following several comments are just moved code - would rather not edit it while moving it (so that git has a change of seeing the moved chunk) Line 156: > docs since this is a public header Done. I didn't doc the individual methods because they're obvious implementations of the iterator interface. 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? Done Line 426: KuduScanBatch::Data::~Data() { > same Done Line 442: addresses into absolute one > this is a bit cryptic. How about: First, rewrite the relative offsets into moved code Line 465: > extra lines Done Line 470: KuduRowResult > why still use KuduRowResult here? Done Line 476: Extracted 0 rows > more info or remove (or increase VLOGness) moved code 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 Done 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 Done 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 Done -- 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
