Todd Lipcon has posted comments on this change. Change subject: KUDU-1600 (part 1): bump to CFile version 2 ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/5678/1/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: PS1, Line 80: 2_t len = > Is the case of parsed_len == 0 stopped representing a corrupted block? I u I don't think it's really problematic -- it'll just feed the empty block to the underlying encoding's decoder, and that will then notice that it's too short and fire a more specific Corruption error. PS1, Line 144: AndParseHeader()); > nit: consider to be more explicit here: Done http://gerrit.cloudera.org:8080/#/c/5678/2/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: PS2, Line 71: const char kMagicStringV1[] = "kuducfil"; : const char kMagicStringV2[] = "kuducfl2"; : const int kMagicLength = 8; > What about: meh... I think it's easy enough to look at them and see that they're the same length, and with the constant right above it, it's pretty clear that they are supposed to have the same one. I like compile_asserts for things like checking the sizeof() a particular struct, where you are afraid someone might make some well-meaning change which breaks some previous assumption on sizes, but here it's visible what's going on and if anyone made such a change it would break pretty immediately when you ran any test. -- To view, visit http://gerrit.cloudera.org:8080/5678 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I13999de3d406fb184cea3e6e8d52ffd80b8d8ee3 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
