Mike Percy has posted comments on this change. Change subject: Reduce arguments in file Read API ......................................................................
Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h File src/kudu/util/env.h: PS3, Line 355: virtual Status Read(Slice* result) Hmm... I am not sure I understand the purpose of changing the function signatures. That part of this patch doesn't seem like an improvement to me. Since we don't use Slice as an input-output parameter anywhere else in the codebase AFAIK I find this API a little surprising; I would have to open the header file to understand how it works when reading code. It also requires additional setup by the user to put something into the Slice before passing it in, and in the case where we might be reading from a memory-mapped file, we wouldn't necessarily want to use the buffer referenced in result.data; we would likely want to reset the Slice to reference mapped memory for zero-copy. Of course, we can still do that with this API but providing scratch as an optional buffer to use and making Slice an output-only parameter makes that contract more explicit. Same elsewhere. If we want to change the signature of these functions, I'd just advocate for putting scratch before slice in the argument order to conform with the style guide, since scratch is really an input-output parameter (arguably even just an input parameter). -- To view, visit http://gerrit.cloudera.org:8080/6770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3e4df425a7387c28eb428e851a9f8c25167755d4 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
