Adar Dembo has posted comments on this change. Change subject: WIP: KUDU-463. Add checksumming to cfile ......................................................................
Patch Set 13: (5 comments) http://gerrit.cloudera.org:8080/#/c/6630/13/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: Line 56: TAG_FLAG(cfile_verify_checksums, experimental); > Done. I am not sure I am clear on the difference between experimental and e I think the main difference is that experimental is for things we don't necessarily want to draw attention to, while evolving is for something that we want people to use, but retain the right to adjust how it's configured in the future. Line 137: Status CFileReader::ReadMagicAndLength(uint64_t offset, Slice* slice) { > I only kept it for the sake of the TRACE_EVENT1. Can remove it. I think every caller already has a TRACE_EVENT() of some kind in it, so we don't need to retain this purely for that. Line 438: } > This is mainly to prevent data_size -= checksum_size from becoming negative I understand, but what exactly does it mean for a ReadBlock() call to provide ptr.size() < 4? Is that impossible when hasChecksums() is true? http://gerrit.cloudera.org:8080/#/c/6630/13/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: Line 67: TAG_FLAG(cfile_write_checksums, experimental); > I describe the defaults a bit in the patch summary. Think I should add a mo No, my reading skills aren't as good as they should be. Sorry about that. But, since we want people to try this feature out, I think evolving is the better tag choice. You could check with Todd/Mike for a second opinion. Line 263: // Prepend a footer checksum. > I saw the same thing but don't have a great answer. I can change it if you If you can't prove to yourself that NOT using WriteRawData() is more correct, then change it. -- To view, visit http://gerrit.cloudera.org:8080/6630 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
