Todd Lipcon has posted comments on this change. Change subject: KUDU-1600 (part 1): bump to CFile version 2 ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/5678/1/docs/design-docs/cfile.md File docs/design-docs/cfile.md: Line 30: <magic>: the string 'kuducfl2' > Maybe we should have both here, with a note about how they enable two "vers Done http://gerrit.cloudera.org:8080/#/c/5678/1/src/kudu/cfile/cfile.proto File src/kudu/cfile/cfile.proto: Line 33: message CFileHeaderPB { > Hmm, but don't we need to retain major/minor version in the definition in o Right, if we now open and read a file that has these fields, they'll just be skipped by the new reader. I removed them to save a few bytes of memory since every CFileReader instantiates and retains a header. Given that the values never had any meaning, I don't find it particularly sad that we won't see the output in the various 'dump' tools. http://gerrit.cloudera.org:8080/#/c/5678/1/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: PS1, Line 79: strlen(kMagicStringV2)) > Would be nice to have a compile-time assert that kMagicStringV1 and kMagicS we can't do a compile assert because strlen() cannot be evaluated at compile time. I'll introduce a new constant Line 81: return Status::Corruption("invalid data size"); > Not a huge fan of functions returning a bad status but having also written Done -- 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: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
