Adar Dembo has posted comments on this change. Change subject: WIP: KUDU-463. Add checksumming to cfile ......................................................................
Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/6630/11/src/kudu/util/env.h File src/kudu/util/env.h: Line 375: Slice* result; // Set to the data and length that was read (OUT) Since there are no short reads, can we omit 'result' altogether? Without short reads, ReadV() either fails (in which case 'entries' is totally invalid) or it succeeds fully (in which case the results of the read is described fully by each 'length' and 'scratch' pair). If the caller needs slices, it can make its own. Alternatively, perhaps we can replace vector<ReadEntry> with vector<Slice>? Each slice describes both 'length' and 'scratch', and would be used as input when constructing the iov internally. In either case, ReadV() won't modify the vector and so it should be passed by const ref to avoid needless copies. An alternative would to be pass it by value and move it, but I think the caller still needs access after ReadV(), and it would lose access if move were used. Given the amount of discussion around this interface, I really think this part of the patch should be split out into its own gerrit change, partly to ease code review and partly so the new git commit can more fully document the design trade-offs taken. -- To view, visit http://gerrit.cloudera.org:8080/6630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
