Skip to content

feat(storage): add option to disable checksums and improve robustness of full_object_checksum validation#17665

Merged
chandra-siri merged 19 commits into
googleapis:mainfrom
chandra-siri:feat/appendable-checksum-robustness
Jul 10, 2026
Merged

feat(storage): add option to disable checksums and improve robustness of full_object_checksum validation#17665
chandra-siri merged 19 commits into
googleapis:mainfrom
chandra-siri:feat/appendable-checksum-robustness

Conversation

@chandra-siri

@chandra-siri chandra-siri commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

This PR introduces performance options and robustness improvements to the asynchronous append operations in AsyncAppendableObjectWriter.

Key Changes:

  1. Disable Checksums for Performance:

    • Added the enable_checksum optional parameter (default: True) to AsyncAppendableObjectWriter.append().
    • When set to False, the writer skips calculation of chunk-level CRC32C checksums in _WriteResumptionStrategy, improving append throughput.
    • Updated unit tests to verify that chunk-level checksums are omitted when disabled.
  2. Robustness of Finalization Checksum Validation:

    • Implemented type checking (isinstance(int)) and range checking ([0, 2**32-1]) for full_object_checksum in finalize().
    • Wrapped stream receiving and response processing inside a try...finally block. This guarantees that the underlying stream is closed and local writer state variables are reset on mismatch exceptions/errors, preventing stream leaks.
    • Simplified the finalization request building code into a clean if-elif-else format.
    • Added corresponding unit tests verifying validation error exceptions and proper stream cleanup.

@chandra-siri chandra-siri requested a review from a team as a code owner July 8, 2026 14:02
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@chandra-siri chandra-siri added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 8, 2026
@chandra-siri

Copy link
Copy Markdown
Contributor Author

review / submit after #17658

@snippet-bot

snippet-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@chandra-siri chandra-siri changed the title feat(storage): support full_object_checksum and add option to disable checksums in AsyncAppendableObjectWriter feat(storage): add option to disable checksums and improve robustness of full_object_checksum validation Jul 9, 2026
@chandra-siri chandra-siri removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 9, 2026
@chandra-siri

Copy link
Copy Markdown
Contributor Author

/gemini review

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

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.

Code Review

This pull request introduces an enable_checksum parameter to control CRC32C checksum calculations during async object writes, adds validation for full_object_checksum in finalize(), and ensures proper stream cleanup. Key feedback includes removing undocumented parameters from the download_ranges docstring, wrapping both send() and recv() in the try...finally block for robust cleanup, and explicitly rejecting boolean values in the full_object_checksum type check along with adding a corresponding unit test.

@chandra-siri chandra-siri enabled auto-merge (squash) July 9, 2026 15:26
@chandra-siri chandra-siri merged commit a5a717d into googleapis:main Jul 10, 2026
30 checks passed
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.

2 participants