Adar Dembo 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 "versions" of cfile? 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 order to read older cfile headers? Or is the point that, there was never any code to read them, so there's no point in defining them? Is that true across the board? Even for the CLI tool, which might dump a DebugString() of the PB? 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 kMagicStringV2 are the same length. Or to use the right one here dependign on the cfile_version. Line 81: return Status::Corruption("invalid data size"); Not a huge fan of functions returning a bad status but having also written to output parameters. Can you fix that? -- 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-HasComments: Yes
