HDDS-15115. CrcUtil/CrcComposer should not throw IOException for non-IO.#10139
Conversation
|
Thanks @szetszwo for working on this. The changes look good to me. I just left a few minor comments. |
@Russole , thanks for reviewing this! But it seems you have not yet posted your comments. Could you check? |
| // CRC corresponding to the actual cellSize boundary. | ||
| assertThrows(IllegalStateException.class, | ||
| () -> digester.update(crcsByChunk[1], CELL_SIZE), | ||
| "stripe"); |
There was a problem hiding this comment.
Nit: assertThrows third argument is the assertion failure message, not an assertion on the exception message. If needed, we can capture the exception and check ex.getMessage() explicitly.
| "stripe"); | |
| IllegalStateException ex = assertThrows(IllegalStateException.class, | |
| () -> digester.update(crcsByChunk[1], CELL_SIZE)); | |
| assertTrue(ex.getMessage().contains("stripe")); |
|
|
||
| assertThrows(IllegalArgumentException.class, | ||
| () -> digester.update(crcBytesByChunk, 0, 6, CHUNK_SIZE), | ||
| "length"); |
There was a problem hiding this comment.
Same here, I think assertThrows third argument is only the assertion failure message, not a check on the thrown exception message.
| "length"); | |
| IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, | |
| () -> digester.update(crcBytesByChunk, 0, 6, CHUNK_SIZE)); | |
| assertTrue(ex.getMessage().contains("length")); |
| int polynomial = CrcUtil.getCrcPolynomialForType(type); | ||
| return new CrcComposer( | ||
| polynomial, | ||
| org.apache.hadoop.ozone.client.checksum.CrcUtil.getMonomial(bytesPerCrcHint, polynomial), |
There was a problem hiding this comment.
Optional nit, unrelated to this change: we could use CrcUtil.getMonomial(...) here for consistency with the previous line.
| org.apache.hadoop.ozone.client.checksum.CrcUtil.getMonomial(bytesPerCrcHint, polynomial), | |
| CrcUtil.getMonomial(bytesPerCrcHint, polynomial), |
|
Sorry @szetszwo, I forgot to submit the review comments. Submitted now. Thanks for the reminder! |
|
@Russole , Good catch on the problems! Just have pushed a fix. Please take a look. |
|
Thanks @szetszwo for the updates. The changes look good to me. LGTM! |
2 participants
What changes were proposed in this pull request?
Similar to HADOOP-19035, CrcUtil and CrcComposer should throw specific exceptions for non-IO cases
What is the link to the Apache JIRA
HDDS-15115
How was this patch tested?
Copied tests from Hadoop.