Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8860 )
Change subject: design-docs: improve cfile.md ...................................................................... Patch Set 2: (18 comments) http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md File docs/design-docs/cfile.md: http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@18 PS1, Line 18: RowSet](tablet.md) each column and DeltaFile will : map to a single CFile. In addit > maybe: "in a separate CFile" to make the distinction from when it's stored Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@24 PS1, Line 24: ter > remove 'the' Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@23 PS1, Line 23: o-one with a file : in the filesystem. The mapping of CFiles to the filesystem is determined by the : [BlockManager](../../src/kudu/fs/block_manager. > Maybe it's be clearer to mention that a CFIle is a logically a unit with he I don't think that would make this section clearer; this is all about the relationship between a CFile and the lower-level BlockManager. The bigginning/middle/end is discussed below in 'File Format'. http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@64 PS1, Line 64: | field | width (bytes) | notes | > https://github.com/apache/kudu/commit/f82cf6918c00dff6aecad5a6b50836b7eb5db Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@64 PS1, Line 64: | field | width (bytes) | notes | > Maybe add a version history with a super short blurb and commit hashes? Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@64 PS1, Line 64: > Would it be nice to briefly doc why do we need to have two versions of CFil I added a link to the commit, which goes into good detail. http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@76 PS1, Line 76: ersion | string | : | --- | --- | > does this necessarily have to be in this file, if there are no limits to th Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@76 PS1, Line 76: rsion | string | : | --- | --- | : | CFile v1 | `kuducfil` | : | CFile v2 > It'd also be nice to clarify the behavior if a value overflows the buffer. I've added the phrase 'best-effort size limited', which hopefully makes this clear. If you think it's vague I can expound, though. http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@86 PS1, Line 86: > It's not. Ack http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@113 PS1, Line 113: > apparently this is deprecated? See common.proto I've added a note on how it's used internally. http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@133 PS1, Line 133: : ### Plain Encodin > Do we actually have good descriptions of these encodings, though? I've added descriptions of all encodings, because I personally feel that it's a useful reference. http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@170 PS1, Line 170: | --- | > again, should this be here? Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@170 PS1, Line 170: --- | : | number of elements | : | ordinal position | : | restart interval | : | unused | : > Is concurrent_btree.h the same btree? I was under the impression that conc Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@199 PS1, Line 199: un-length Encodi > Yeah. I think the diagram may be overkill, but I think we should mention th Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@212 PS1, Line 212: : ### Group Varint Frame-of-Refe > interspersed, iirc Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@217 PS1, Line 217: : Starts with a header of four [group-varint](group-varint) encoded > +1 Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@225 PS1, Line 225: > not your fault, but typo. Done http://gerrit.cloudera.org:8080/#/c/8860/1/docs/design-docs/cfile.md@260 PS1, Line 260: | | > This is the first time mention 'incompatible_features'. And do not see it i Done -- To view, visit http://gerrit.cloudera.org:8080/8860 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I770028bba3f7a49c96f32893c285221c84be39ce Gerrit-Change-Number: 8860 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Sat, 20 Jan 2018 03:06:32 +0000 Gerrit-HasComments: Yes
