Adar Dembo has posted comments on this change. Change subject: env: Always read fully when reading files ......................................................................
Patch Set 1: (3 comments) Two high level comments: - In many places, env_util.h was only included for ReadFully(). Wherever you're replacing ReadFully() with Read(), could you check if that was the last thing that requires env_util.h, and if so, remove it? - With short reads out of the picture, I wonder if we can simplify the Read() API a bit. As per the comment I left in your checksumming patch, having both the scratch/len and the slice seems redundant; one or the other should suffice to fully describe the read input and output. http://gerrit.cloudera.org:8080/#/c/6758/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: PS1, Line 260: *result = Slice(scratch, 0); A calling convention we like to adhere to is: if the function returns a bad Status, don't modify any OUT parameters. To apply that here, use a local Slice and set *result only when you know you're going to return success. PS1, Line 269: return Status::IOError( : Substitute("Can't short read great than or equal to the $0 bytes requested", length)); Since this is test-only, perhaps just CHECK fail here? Line 288: *result = Slice(scratch, result->size() + r); Given the above calling convention, you should only need to set up the results slice once, when all the reading is done. -- To view, visit http://gerrit.cloudera.org:8080/6758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I735cd4cfca3c355226266ef4f0fdb57bf59dfe69 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
