Repository navigation
HDDS-16834. Fix COMPOSITE_CRC when a block contributes no CRC bytes - #11499
Open
jojochuang wants to merge 5 commits into
Open
jojochuang wants to merge 5 commits into
jojochuang wants to merge 5 commits into
Conversation
Port the HDFS-17992 cursor-based block checksum walk so empty blocks do not misalign composite CRC assembly, and skip zero-length blocks with no chunks. Add replicated and EC regression tests. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Ic3c039be4b464e44e4e44fd8876ba0b44f9165f2
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The all-empty real execution path still throws before reaching the new fallback.
1 open finding
What changed in this PR
Fixes composite CRC calculation when empty blocks contribute no checksum bytes.
Changes:
- Skips empty block metadata safely.
- Uses a cursor for composite CRC bytes.
- Adds replicated and EC checksum tests.
| File | Description |
|---|---|
BaseFileChecksumHelper.java |
Handles empty blocks during checksum composition. |
TestFileChecksumHelper.java |
Adds empty-block checksum coverage. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Return the empty-key checksum before constructing CrcComposer when every block is skipped, so checksumType is not required. Add a test that exercises checksumBlock on empty blocks without pre-setting checksum metadata. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I701c0e01ed34de7750433b5f51b162012cc7177a
Resolve TestFileChecksumHelper conflict: keep COMPOSITE_CRC empty-block tests and align helpers with String volume/bucket constructors on master. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Id28507b587a57effd6f7fb7cd4e58d6d20625c1a
rich7420
reviewed
Oct 13, 2026
Detect the all-empty case via blockChecksumBuf.getLength() == 0 before constructing CrcComposer, and restore full block.getLength() composition so partial requested lengths do not produce negative update sizes. Add tests for partial length across two blocks. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: If21b9b076e968c9ad307163b5b4e1a3d9f2d819d
jojochuang
commented
Oct 15, 2026
jojochuang
left a comment
jojochuang
left a comment
Contributor
Author
There was a problem hiding this comment.
test self-review
Use the first non-empty block for CrcComposer size hint, add a leading empty-block test, and replace assertNotEquals(null, …) with assertNotNull. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I697503e632d55fc53b23c3190de01c9b618a837b
rich7420
reviewed
Oct 17, 2026
| return new MD5MD5CRC32GzipFileChecksum(0, 0, fileMD5); | ||
| } | ||
|
|
||
| private FileChecksum makeFinalResult() throws IOException { |
Contributor
There was a problem hiding this comment.
Please handle empty results before the mode switch. Skipping every block leaves checksumType null and throws in MD5 mode.
Suggested change
| private FileChecksum makeFinalResult() throws IOException { | |
| private FileChecksum makeFinalResult() throws IOException { | |
| if (blockChecksumBuf.getLength() == 0) { | |
| // Skipped empty blocks may leave checksumType unset. | |
| return makeEmptyFileChecksum(); | |
| } |
Please also test RATIS/EC × both modes through checksumBlock(); the MD5 fake pre-sets the type.
Comment on lines
383
to
+390
| FileChecksum makeCompositeCrcResult() throws IOException { | ||
| if (blockChecksumBuf.getLength() == 0) { | ||
| // Every block is empty, e.g. a zero-length key whose writer died | ||
| // after allocating a block. Treat it like a key without blocks. | ||
| // Detect this before constructing CrcComposer because skipped empty | ||
| // blocks never set checksumType. | ||
| return makeEmptyFileChecksum(); | ||
| } |
Contributor
There was a problem hiding this comment.
Please remove this guard after moving it to makeFinalResult().
Suggested change
| FileChecksum makeCompositeCrcResult() throws IOException { | |
| if (blockChecksumBuf.getLength() == 0) { | |
| // Every block is empty, e.g. a zero-length key whose writer died | |
| // after allocating a block. Treat it like a key without blocks. | |
| // Detect this before constructing CrcComposer because skipped empty | |
| // blocks never set checksumType. | |
| return makeEmptyFileChecksum(); | |
| } | |
| FileChecksum makeCompositeCrcResult() throws IOException { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BaseFileChecksumHelper#makeCompositeCrcResultto walk block checksum bytes with a cursor and skip blocks that contribute no composite CRC bytes (same approach as HDFS-17992).Jira
https://issues.apache.org/jira/browse/HDDS-16834
Test plan
mvn -pl :ozone-client test -Dtest=TestFileChecksumHelper -DskipShade -DskipRecon -DskipDocs./hadoop-ozone/dev-support/checks/checkstyle.shGenerated-by: Cursor (Auto)
Made with Cursor