Grant Henke has posted comments on this change. Change subject: WIP: KUDU-463. Add checksumming to cfile ......................................................................
Patch Set 15: (14 comments) http://gerrit.cloudera.org:8080/#/c/6630/15/docs/design-docs/cfile.md File docs/design-docs/cfile.md: Line 31: <header length>: 32-bit unsigned integer length delimiter > specify whether this includes the checksum or just the following PB Done Line 33: <checksum>: An optional Crc32 checksum of the entire header > does this include the magic? Done Line 43: <checksum>: An optional Crc32 checksum of the entire footer > same - including the footer, magic, and pb len? Done Line 211: When checksums for the header, data, and footer are written in the CFile > nit: add comma at end of line Done http://gerrit.cloudera.org:8080/#/c/6630/15/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 223: opts.storage_attributes.compression = compression; > oh, that's interesting, we forgot to actually use the compression here? I think so. I noticed it while doing my testing. Line 336: uint8_t orig = data.mutable_data()[corrupt_offset]; > Nit: could do data.data()[corrupt_offset] here I think (though it may mean Done http://gerrit.cloudera.org:8080/#/c/6630/15/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: Line 141: // Parse Footer first to find unsupported features. > does this mean that an old server will have problems with checksummed heade It shouldn't because the length doesn't include the checksum. Line 146: if (PREDICT_FALSE(footer_->incompatible_features() > IncompatibleFeatures::ALL)) { > '>' strikes me as strange here vs doing & ~IncompatibleFeatures::ALL I put a comment on a similar older review and wasn't sure if I should change it. Here was the response: > Your check is technically more correct since if there are bits in the middle > that are not valid but are set, it will check that. I assumed incompatible > feature bits would be used sequentially. Especially since we output > "supported" as an integer in the log message below. Line 150: IncompatibleFeatures::ALL)); > instead of ALL, maybe a better name would be 'SUPPORTED'? Done Line 239: if (footer_size >= file_size_ - kMagicAndLengthSize) { > do we want to have some kind of max size here? am afraid a flipped bit in a ParseMagicAndLength has a max size check: if (len > kMaxHeaderFooterPBSize) { return Status::Corruption("invalid data size"); } Line 416: uint32_t checksum_size = sizeof(uint32_t); > you have this in a few places. Maybe better to just have a global constant Done http://gerrit.cloudera.org:8080/#/c/6630/15/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: Line 230: incompatible_features = IncompatibleFeatures::CHECKSUM; > maybe |= here so it doesn't need to change when we add some other feature Done Line 500: // Calculate and append a checksum data checksum. > typo? Done http://gerrit.cloudera.org:8080/#/c/6630/15/src/kudu/util/crc-test.cc File src/kudu/util/crc-test.cc: Line 50: const uint64_t expected_crc = 0xa9421b7; // Known value from crcutil usage test program. > nit: kExpectedCrc Done -- 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: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
