HDDS-11482. EC Checksum throws IllegalArgumentException because the buffer limit is negative #7230
Conversation
…uffer limit is negative
There was a problem hiding this comment.
Thanks @aswinshakil for the fix, Looks correct to me, I also verified the patch against the unit tests that I had written to repro this and it works.
| long remainingChunkSize = Math.min(keySize, chunkSize); | ||
| while (byteWrap.hasRemaining() && remainingChunkSize > 0) { | ||
| final int checksumData = byteWrap.getInt(); | ||
| blockCrcComposer.update(checksumData, Math.min(bytesPerCrc, remainingChunkSize)); |
There was a problem hiding this comment.
If I understand correctly, this should handle the case when chunk size is not a multiple of bytes.per.crc right ? as we have removed the code pertaining to this part above.
There was a problem hiding this comment.
I am not sure I understand this section.
Also, is this routine called on a block? If an EC key has multiple blocks, then first the checksums are combined for block 1 in a call to computeMd5Crc and then a subsequent call will be made for block 2 to combine its checksums into the value for block 1? How does the keySize work then across multiple blocks?
There was a problem hiding this comment.
@sodonnel Thanks for the catch. It should be blockSize instead of the keySize as we are calculating checksum per block here.
|
@sadanand48 @sodonnel Can you take another look at this? Thank you! |
aswinshakil
requested review from
sadanand48 and
sodonnel
and removed request for
sodonnel
What changes were proposed in this pull request?
The parity checksum bytes calculation for the EC stripe checksum should be based on each stripe's first chunk's length. But right now it is calculated only on the first stripe's chunk length which results in incorrect results and, in many cases causes the byte buffer limit to become negative. Further, the checksum calculation has been simplified in this patch.
The checksum calculation for the last stripe is a little tricky, We have two cases for it.
For EC, We store the stripe checksum for the entire stripe in replica index 1 and the parity blocks. We use this stripe checksum to calculate the checksum during reading. The chunks we get during checksum calculation will always be from replica index 1 or parity blocks. And the chunk length will always be the stripe's first chunk's length.
Hence, we don't get the length of the last chunk which may be partial in most of the cases. We calculate the last chunk's size from the block size.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11482
How was this patch tested?
Updated the existing test.
Test fails without the fix:https://github.com/aswinshakil/ozone/actions/runs/11002573679/job/30550687378
Test passed with the fix: https://github.com/aswinshakil/ozone/actions/runs/11002650413/job/30551032116