Adar Dembo has posted comments on this change. Change subject: Reduce arguments in file Read API ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env.h File src/kudu/util/env.h: Line 376: // Read up to "result.size" from the file starting at "offset". Why "up to"? Isn't guaranteed to read exactly that amount, or fail? PS3, Line 380: // This method will internally retry on EINTR and "short reads" in order to : // fully read the requested number of bytes. In the event that it is not : // possible to read exactly 'length' bytes, an IOError is returned. This note should probably exist in the other Read() implementations too. Or (if you don't want to copy it repeatedly) it can be generalized somewhere and then linked to from the various Read() implementations. PS3, Line 504: up to Same question about "up to". http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 278: req, offset)); Nit: I think result->length() is actually more correct here. Yes, the EOF occurred on one of potentially several pread() calls, but the caller doesn't know how many calls were made, and so the caller would expect the error to describe the logical read itself. Line 308: *result = Slice(result->mutable_data(), r); Wouldn't result->truncate(r) be simpler? http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util-internal.cc File src/kudu/util/pb_util-internal.cc: PS3, Line 49: if (result.data() != buffer_.get()) { : memcpy(buffer_.get(), result.data(), result.size()); : } I don't think this is possible anymore, is it? http://gerrit.cloudera.org:8080/#/c/6770/3/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: Line 183: Status NonShortRead(T* file, uint64_t offset, Slice* result); Is the indirection via NonShortRead() still needed? Or can we just call reader->Read() directly in ValidateAndReadData()? -- 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: Tidy Bot Gerrit-HasComments: Yes
