Mike Percy has posted comments on this change. Change subject: KUDU-1377 (part 2): Add version 2 of protobuf container file ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/2815/1//COMMIT_MSG Commit Message: Line 13: field (huge number) can look like a truncated file. > Nit: s/can/could/ (to agree with the "did not have" tense) What is this, journalism class? ;) http://gerrit.cloudera.org:8080/#/c/2815/1/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: Line 270: EOF here is acceptable: it means we're : // out of PB entries. > This isn't true, is it? If we were out of entries, we would have noticed on yeah, this got copy / pasted but is wrong, will remove it Line 372: RETURN_NOT_OK_PREPEND(ReadAndCompareChecksum(reader, file_size, &tmp_offset, { header }), > This makes for good code sharing between v1 and v2, but ideally a v2 contai For performance reasons? I think readahead should probably take care of the IO / seek overhead, typically. The problem is that we don't know whether this field needs to be there before we read the version so doing contortions to try to optimize for that case doesn't really seem to be worth it, since this only happens once per file, when opening it. Line 584: initialized_ = true; > Isn't initialized_ already true? Doesn't Reopen() have to happen after Init No, Init() should not be called before Reopen(). Init() writes a header to the file. Line 814: CHECK(initialized_); > Not DCHECK? i guess we should use DCHECK, since it's a programming error. http://gerrit.cloudera.org:8080/#/c/2815/1/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: Line 309: bool initialized_; : bool closed_; > I think only three states are actually supported, right? Would be cleaner ( Sure, I can do that. -- 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-HasComments: Yes
