[kudu-CR] KUDU-1259: new scanner API with an encapsulated Batch object

["Todd Lipcon (Code Review)" <ge...@ip-10-146-233-104.ec2.internal>]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1259: new scanner API with an encapsulated Batch object
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/1562/4/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 1051: //   for (KuduRowResult row : batch) {
> does that work even though *it returns a temporary rather than a reference?
Left this as is, since a reference to a temporary seems wrong, and the KuduRowResult has a trivial copy constructor so it should be optimizable anyway.


Line 1067: class KUDU_EXPORT KuduScanBatch {
> If you intend to follow the C++ container style, it would be great to defin
Done


http://gerrit.cloudera.org:8080/#/c/1562/4/src/kudu/client/row_result.h
File src/kudu/client/row_result.h:

Line 38: w
> Circling back to my other comment about an inline class. Since this class i
I like this idea. Made the rename, and left a typedef of the old name to preserve API compatibility. It breaks the ABI, but I think that's OK given the beta status.


Line 89:   friend class KuduScanBatch;
> Good idea... let me see if this is possible while also retaining API compat
I did this. However, the friend declaration is still necessary -- inner classes don't get special access to their enclosing class in C++03. I believe C++11 changes this.


-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Martin Grund <mg...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes