-
Notifications
You must be signed in to change notification settings - Fork 153
Add CRC64NVME checksum support #633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5d990bd
3475b5a
d169f21
2475c99
e49a372
6735441
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,16 +20,20 @@ use std::str::FromStr; | |
|
|
||
| #[allow(non_camel_case_types)] | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| #[non_exhaustive] | ||
| /// Enum representing checksum algorithm supported by S3. | ||
| pub enum Checksum { | ||
| /// SHA-256 algorithm. | ||
| SHA256, | ||
| /// CRC64-NVME algorithm. | ||
| CRC64NVME, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is technically a breaking change because this enum was (erroneously IMO) not marked as But @kdn36 you should definitely mark this enum as
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we would have to wait for the next breaking object store release. We could discuss making such a change and quickly release 0.14 (next breaking change) if needed |
||
| } | ||
|
|
||
| impl std::fmt::Display for Checksum { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match &self { | ||
| Self::SHA256 => write!(f, "sha256"), | ||
| Self::CRC64NVME => write!(f, "crc64nvme"), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -40,6 +44,7 @@ impl FromStr for Checksum { | |
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s.to_lowercase().as_str() { | ||
| "sha256" => Ok(Self::SHA256), | ||
| "crc64nvme" => Ok(Self::CRC64NVME), | ||
| _ => Err(()), | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked and indeed CRC64NVME seems to be the default suggestion:
https://docs.aws.amazon.com/AmazonS3/latest/userguide/checking-object-integrity.html
Seems like it was added to the official SDK about a year ago:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to change the enum, perhaps we can add all the other supported checksums as well (we don't have to actually support them, but we can minimize API churn)
I think that would mean adding the following variants (with comments that they aren't yet supported)
CRC32
CRC32C
SHA1
MD5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb If the enum is marked as
#[non_exhaustive]as I recommended above then adding new variants isn't a breaking change. I personally don't think it's a good idea to add variants which aren't actually supported.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking it as
#[non_exhaustive]would also be fine(my rationale to add Variants that are not supported was to 🎣 for help supporting them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. Changed to
#[non-exhaustive]. If this goes through, adding more algorithm variants should be straightforward.