Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12109 )

Change subject: tablet: add support for diff scan iterator
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12109/2/src/kudu/tablet/diff_scan-test.cc
File src/kudu/tablet/diff_scan-test.cc:

http://gerrit.cloudera.org:8080/#/c/12109/2/src/kudu/tablet/diff_scan-test.cc@89
PS2, Line 89:   opts.snap_to_exclude = snap1;
Technically you needn't use a snap_to_exclude either; just 
include_deleted_rows=true is enough to get multiple ghosts.


http://gerrit.cloudera.org:8080/#/c/12109/2/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:

http://gerrit.cloudera.org:8080/#/c/12109/2/src/kudu/tablet/tablet.h@205
PS2, Line 205:   // Note: the 'projection' and 'io_context' fields of the 
'opts' struct will
             :   // be ignored and overwritten in the copy of the 'opts' struct 
used by the
             :   // returned iterator because it holds its own instances of 
those objects.
One thing you could do to make this more user friendly is to drop the 
'projection' arg and have NewRowIterator() make a copy of opts.projection and 
store that copy in the iterator. So the caller would still need to at least 
make the projection on the stack, but NewRowIterator() would copy it, so the 
stack copy could be dropped after the call.



--
To view, visit http://gerrit.cloudera.org:8080/12109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9aa0ce2276bdd37688e7a91a2efcf49a8f802eb5
Gerrit-Change-Number: 12109
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Grant Henke <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 20 Dec 2018 16:39:07 +0000
Gerrit-HasComments: Yes

Reply via email to