feat: add default checksums for appendable writer #13379
Conversation
Summary of ChangesHello @krishnamd-jkp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces default checksumming capabilities for gRPC appendable writers, enhancing data integrity during object writes. It also includes important documentation regarding the performance implications of automatic checksum computation and provides a mechanism to disable it. A temporary measure has been implemented to disable checksums for "takeover" writers, with a clear indication for future development. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 default checksumming for appendable writers in the gRPC client. It introduces logic to calculate and send checksums for chunks and for the full object, while also providing an option to disable this for performance reasons. A temporary flag is added to disable checksums for takeover writers, with a TODO to implement it later. The changes are well-documented and tested.
My review includes a suggestion to refactor how checksum settings are passed to the gRPCAppendBidiWriteBufferSender. Instead of using a function that is called on every send, I recommend storing the immutable settings as direct fields on the struct for better clarity and minor performance improvement.
| finishWrite bool | ||
| // checksumAttrs contains all parameters required from writer in senders. | ||
| // helps avoid piping in writer to the individual senders. | ||
| type checksumAttrs struct { |
There was a problem hiding this comment.
thought: If we are adding a significant amount of code related to checksums, it might make sense to move it to a different file now, rather than later when it would become even more complex.
There was a problem hiding this comment.
I think with the refactor, the checksum related stuff boils down to 2 structs and a function. I feel it's okay to keep this in the same file
2 participants
No description provided.