Mike Percy has posted comments on this change. Change subject: KUDU-1377 (part 2): Add version 2 of protobuf container file ......................................................................
Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/2815/1//COMMIT_MSG Commit Message: Line 13: field (huge number) can look like a truncated file. > Always. I honestly don't remember learning these rules. I probably didn't do all my assigned reading in grammar class? Line 15: checksum. This may compromise version-specific logic. > Nit: s/may/could/ (same reason) Done http://gerrit.cloudera.org:8080/#/c/2815/1/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: Line 338: // Read and parse the protobuf container file-level header documented above. > It's actually documented in pb_util.h though. Done Line 372: RETURN_NOT_OK_PREPEND(ReadAndCompareChecksum(reader, file_size, &tmp_offset, { header }), > yea I think given it's not perf critical here, err on the side of less code I ended up doing this optimization, because I noticed that I could halve the number of reads for the more common path (reading PB records) and once I did that then it was easy to reuse it here. http://gerrit.cloudera.org:8080/#/c/2815/1/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: Line 156: // "data length" and "data" fields. In versions >= 1, this is only a > Nit: In version 2+ (to be consistent with "version 2+ only" elsewhere). Done Line 162: // A valid container must have at least one protobuf message, the first of > Nit: since the contents of a container file is now referred to as a "record Done Line 180: data : // size fields > Nit: earlier you referred to it as "data length", now "data size". Pick one Done Line 225: // In short, no version of the file format will not detect truncation of entire > Will NOT detect? Don't you mean "will detect"? Done Line 257: specified version > Where is the version specified, exactly? clarified this whole section Line 371: // Return current read offset. File must be open. > Nit: change to "Open() must be called first" (or change version() to say "F Done Line 375: bool initialized_; : bool closed_; > See my above comment about a tri-state enum. Done -- To view, visit http://gerrit.cloudera.org:8080/2815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I239c7b99a55a74a6a658ff418830c1f9be13eaa6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
