Huginn-kio commented on PR #8661: URL: https://github.com/apache/hbase/pull/8661#issuecomment-5795789064
> The name of the property here is "onDiskDataSizeWithHeader" so it should record the proper block size on disk. If we apply this change, we won't be recording the actual block size on disk. Thanks for the comment. I think the distinction between the two header fields was not clear in the PR. For an HFile block: - `onDiskSizeWithoutHeader` is the physical on-disk block size excluding the header and including trailing checksum bytes (`D + C`). - `onDiskDataSizeWithHeader` is the header plus on-disk data before the checksum region (`H + D`). This is also documented by `Writer#putHeader()`. `cloneOnDiskBufferWithHeader()` returns exactly `H + D` and does not copy `onDiskChecksum`. Therefore, changing `onDiskDataSizeWithHeader` to `onDiskBlockBytesWithHeader.size()` does not lose the physical block size. The physical size remains represented by `onDiskSizeWithoutHeader`; this change only makes the checksum boundary metadata consistent with the buffer. The metadata has an extra section the length of the checksum compared to the actual buffer, causing the buffer boundary and header metadata to be inconsistent, which led to out-of-bounds access when we implemented fast-path decompression. Whether on cache-on-read or cache-on-write, the actual cached buffer does not contain a checksum. I agree with your point about needing to be compatible with already cached blocks, but I think this semantics ultimately still needs to be revised. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
