Skip to content

feat(storage): add full object checksum validation for grpc flow#13265

Open
Dhriti07 wants to merge 3 commits into
add-cumulative-hasherfrom
add-cumulative-hasher-grpc-flow
Open

feat(storage): add full object checksum validation for grpc flow#13265
Dhriti07 wants to merge 3 commits into
add-cumulative-hasherfrom
add-cumulative-hasher-grpc-flow

Conversation

@Dhriti07
Copy link
Copy Markdown
Contributor

Adding full object checksum for grpc flow

Refer to: go/full_checksum_java

@Dhriti07 Dhriti07 requested review from a team as code owners May 25, 2026 15:09
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces cumulative checksum validation for full object reads in the GapicUnbufferedReadableByteChannel. It wraps the existing hasher in a CumulativeHasher when a read starts at offset 0 and triggers validation once the end of the stream is reached. The update includes several new test cases to verify validation behavior across different scenarios, such as multi-chunk reads and ranged reads. Review feedback suggests optimizing the implementation by only instantiating the CumulativeHasher when no read limit is specified, thereby avoiding unnecessary overhead for ranged reads where validation is skipped.

Comment on lines +95 to +97
this.hasher = (req.getReadOffset() == 0)
? new CumulativeHasher(hasher, 0, req.getReadLimit() <= 0 ? OptionalLong.empty() : OptionalLong.of(req.getReadLimit()))
: hasher;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The CumulativeHasher is currently instantiated for any read starting at offset 0, even if a readLimit is provided. However, the test case validateCumulativeChecksum_skippedForRangedRead demonstrates that validation is skipped when a limit is present. To avoid unnecessary object allocation and wrapping overhead for ranged reads, the CumulativeHasher should only be instantiated when both the offset is 0 and no limit is specified (i.e., a full object read).

Suggested change
this.hasher = (req.getReadOffset() == 0)
? new CumulativeHasher(hasher, 0, req.getReadLimit() <= 0 ? OptionalLong.empty() : OptionalLong.of(req.getReadLimit()))
: hasher;
this.hasher = (req.getReadOffset() == 0 && req.getReadLimit() <= 0)
? new CumulativeHasher(hasher, 0, OptionalLong.empty())
: hasher;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant