feat(storage): adding disabling option for bidi reads#13506
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors CRC32C checksum validation by replacing Hasher.enabled() with Hasher.defaultHasher() and checking against Hasher.noop(). It also updates CumulativeHasher to use the delegate's initial value, handle null values safely, and adds a unit test for disabled validation. The review feedback correctly points out that removing the !(hasher instanceof Hasher.NoOpHasher) check in BaseObjectReadSessionStreamRead leads to unnecessary allocations of CumulativeHasher when checksumming is disabled, and recommends restoring this check.
| this.readId = readId; | ||
| this.hasher = | ||
| (rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher)) | ||
| (rangeSpec.begin() == 0) |
There was a problem hiding this comment.
By removing the !(hasher instanceof Hasher.NoOpHasher) check, a CumulativeHasher is now always allocated and wrapped around the delegate hasher when rangeSpec.begin() == 0, even if checksum validation is disabled (i.e., when hasher is a NoOpHasher). Keeping this check avoids unnecessary object allocation and overhead when checksumming is disabled.
| (rangeSpec.begin() == 0) | |
| (rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher)) |
References
- Prefer lazy initialization over eager initialization for resource-intensive objects if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.
| this.readId = new AtomicLong(readId); | ||
| this.hasher = | ||
| (rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher)) | ||
| (rangeSpec.begin() == 0) |
There was a problem hiding this comment.
By removing the !(hasher instanceof Hasher.NoOpHasher) check, a CumulativeHasher is now always allocated and wrapped around the delegate hasher when rangeSpec.begin() == 0, even if checksum validation is disabled (i.e., when hasher is a NoOpHasher). Keeping this check avoids unnecessary object allocation and overhead when checksumming is disabled.
| (rangeSpec.begin() == 0) | |
| (rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher)) |
References
- Prefer lazy initialization over eager initialization for resource-intensive objects if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.
Add option to disable Bidi Checksum using checksum disable system property