feat(storage): Add full object checksum validation for appendable uploads#16219
feat(storage): Add full object checksum validation for appendable uploads#16219v-pratap wants to merge 16 commits into
Conversation
product-auto-label
Bot
added
the
api: storage
v-pratap
force-pushed
the
fastbyte-full-object-checksum-final
branch
from
3aad347 to
915cef5
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for CRC32C checksum validation during asynchronous uploads, specifically during finalization, resume, and flush operations. It updates the AsyncWriter interface and its underlying connection implementations to accept and verify expected checksums, and enhances HashFunction to support state retrieval and restoration. The review feedback highlights three critical issues: a compilation error in AsyncWriterConnectionImpl::Finalize caused by inconsistent return types in a lambda, and two data races in AsyncWriterConnectionResumed where impl_ is accessed concurrently without holding the appropriate mutex lock.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16219 +/- ##
==========================================
- Coverage 92.23% 92.21% -0.03%
==========================================
Files 2265 2265
Lines 210210 210211 +1
==========================================
- Hits 193894 193845 -49
- Misses 16316 16366 +50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… in appendable uploads
…overloads without default arguments
…ck slice calculations
v-pratap
force-pushed
the
fastbyte-full-object-checksum-final
branch
from
006eeda to
9fca72d
Compare
…tate to avoid offset rewinding and MD5 errors
…heckpoints during a stream resume
| future<StatusOr<google::storage::v2::Object>> Finalize(AsyncToken token); | ||
| future<StatusOr<google::storage::v2::Object>> Finalize( | ||
| AsyncToken token, | ||
| absl::optional<Crc32cChecksumValue> const& expected_checksum); |
There was a problem hiding this comment.
nit: https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs recommends avoiding absl::optional<T> const&.
If possible, we should either pass std::optional<T> by value (if it's a by-value input) or pass const T* (if it would otherwise be passed by reference).
…ose() phases as per revised design
Based on the revised design, full-object checksum verification is exclusively handled server-side at Finalize(), eliminating the need for continuous client-side incremental hashing across the stream. - Removed dynamic hash computation from CreateHashFunction for all async writers. - Removed dead client-side mismatch checking in AsyncWriterConnectionImpl and AsyncWriterConnectionResumed. - Consolidated appendable and standard hash creation to use PrecomputedHashFunction only. - Cleaned up obsolete tests asserting client-side hash mismatch failures.
2 participants
No description provided.