feat(storage): Adding CumulativeHasher wrapper class for Full object …#13239
feat(storage): Adding CumulativeHasher wrapper class for Full object …#13239Dhriti07 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the CumulativeHasher class, which wraps a Hasher to track and validate cumulative CRC32C checksums for full object reads. It also updates the visibility of existing checksum exception constructors in Hasher.java. The review feedback highlights a logic gap where direct calls to hash methods were not being accumulated, potentially leading to incorrect cumulative totals. Additionally, suggestions were made to mark the new exception class as final and to use Locale.US in string formatting for consistency with existing patterns in the codebase.
| @Override | ||
| public Crc32cLengthKnown hash(ByteBuffer b) { | ||
| return delegate.hash(b); | ||
| } | ||
|
|
||
| @Override | ||
| public Crc32cLengthKnown hash(ByteString byteString) { | ||
| return delegate.hash(byteString); | ||
| } | ||
|
|
||
| @Override | ||
| public void validate(Crc32cValue<?> expected, Supplier<ByteBuffer> b) throws ChecksumMismatchException { | ||
| ByteBuffer byteBuffer = b.get(); | ||
| Crc32cLengthKnown actual = delegate.hash(byteBuffer); | ||
| if (actual != null) { | ||
| if (expected != null && !actual.eqValue(expected)) { | ||
| throw new ChecksumMismatchException(expected, actual); | ||
| } | ||
| accumulate(actual); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void validate(Crc32cValue<?> expected, ByteString byteString) throws ChecksumMismatchException { | ||
| Crc32cLengthKnown actual = delegate.hash(byteString); | ||
| if (actual != null) { | ||
| if (expected != null && !actual.eqValue(expected)) { | ||
| throw new ChecksumMismatchException(expected, actual); | ||
| } | ||
| accumulate(actual); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void validateUnchecked(Crc32cValue<?> expected, ByteString byteString) | ||
| throws UncheckedChecksumMismatchException { | ||
| Crc32cLengthKnown actual = delegate.hash(byteString); | ||
| if (actual != null) { | ||
| if (expected != null && !actual.eqValue(expected)) { | ||
| throw new UncheckedChecksumMismatchException(expected, actual); | ||
| } | ||
| accumulate(actual); | ||
| } | ||
| } |
There was a problem hiding this comment.
The CumulativeHasher should accumulate checksums for all data processed, including calls to the hash methods. Currently, hash(ByteBuffer) and hash(ByteString) only delegate without updating cumulativeHash. Updating these methods to accumulate and then having the validate methods leverage them ensures consistency and reduces duplication.
@Override
public Crc32cLengthKnown hash(ByteBuffer b) {
Crc32cLengthKnown actual = delegate.hash(b);
if (actual != null) {
accumulate(actual);
}
return actual;
}
@Override
public Crc32cLengthKnown hash(ByteString byteString) {
Crc32cLengthKnown actual = delegate.hash(byteString);
if (actual != null) {
accumulate(actual);
}
return actual;
}
@Override
public void validate(Crc32cValue<?> expected, Supplier<ByteBuffer> b) throws ChecksumMismatchException {
Crc32cLengthKnown actual = hash(b.get());
if (actual != null && expected != null && !actual.eqValue(expected)) {
throw new ChecksumMismatchException(expected, actual);
}
}
@Override
public void validate(Crc32cValue<?> expected, ByteString byteString) throws ChecksumMismatchException {
Crc32cLengthKnown actual = hash(byteString);
if (actual != null && expected != null && !actual.eqValue(expected)) {
throw new ChecksumMismatchException(expected, actual);
}
}
@Override
public void validateUnchecked(Crc32cValue<?> expected, ByteString byteString)
throws UncheckedChecksumMismatchException {
Crc32cLengthKnown actual = hash(byteString);
if (actual != null && expected != null && !actual.eqValue(expected)) {
throw new UncheckedChecksumMismatchException(expected, actual);
}
}
Adding CumulativeHasher wrapper class for Full object checksum
Refer to: go/full_checksum_java