Return PutResult with an ETag from ObjectStore::put (#4934)
#4944
Conversation
alamb
changed the title
PutResult with an ETag from ObjectStore::put (#4934)
| @@ -243,12 +249,14 @@ impl S3Client { | |||
| } | |||
|
|
|||
| /// Make an S3 PUT request <https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObject.html> | |||
| /// | |||
| /// Returns the ETag | |||
| pub async fn put_request<T: Serialize + ?Sized + Sync>( | |||
There was a problem hiding this comment.
this structure (S3Client) is not public, so this is not a breaking API change in case anyone else was wondering
| let part = (part_idx + 1).to_string(); | ||
|
|
||
| let response = self | ||
| let content_id = self |
There was a problem hiding this comment.
is there a reason this is called content_id here and etag elsewhere? I just found the difference in terminology confusing
There was a problem hiding this comment.
It's because PartId is used across multiple stores, and for some of them this isn't an etag but a part number
| /// Result for a put request | ||
| #[derive(Debug)] | ||
| pub struct PutResult { | ||
| /// The unique identifier for the object |
There was a problem hiding this comment.
Perhaps this could also include a link to what an ETAG is for those who are not familiar
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag perhaps
object_store/src/lib.rs
Outdated
| @@ -850,6 +850,13 @@ impl GetResult { | |||
| } | |||
| } | |||
|
|
|||
| /// Result for a put request | |||
| #[derive(Debug)] | |||
There was a problem hiding this comment.
I always find it annoying when these traits are not derived for simple objects.
| #[derive(Debug)] | |
| #[derive(Debug, Hash, Clone, PartialOrd)] |
There was a problem hiding this comment.
I've added Clone, but I'm not sure about Hash and PartialOrd here
There was a problem hiding this comment.
Clone is probably good enough to start with
| Ok(PutResult { | ||
| e_tag: Some(etag.to_string()), | ||
| }) |
There was a problem hiding this comment.
If you made PutResult have non pub fields, maybe this could be a builder style API like this
OK(PutResult::new().with_etag(etag))| @@ -850,6 +850,13 @@ impl GetResult { | |||
| } | |||
| } | |||
|
|
|||
| /// Result for a put request | |||
| #[derive(Debug)] | |||
| pub struct PutResult { | |||
There was a problem hiding this comment.
I also wonder if we should makd this a non pub field and add an accessor / into_etag() function instead to allow flexibility in adding new fields later without making a breaking API change
However, the existing GetResult https://docs.rs/object_store/latest/object_store/struct.GetResult.html has pub fields so this just follows the same pattern
| .put_request(location, Some(bytes), false, &()) | ||
| .await?; | ||
| Ok(()) | ||
| let e_tag = Some(get_etag(response.headers()).context(MetadataSnafu)?); |
There was a problem hiding this comment.
Is it guaranteed that these services (and their emulators, etc) all return an ETag? I wonder if this should be an error, or if the etag isn't present in the response, this should return Ok(PutResult{ etag: None}) 🤔
There was a problem hiding this comment.
The actual object stores guarantee it is present, the same is technically true of WebDav, but given HTTPStore gets used for more than that I opted to not make it so strict
Which issue does this PR close?
Closes #4934
Rationale for this change
Returning the ETag on ObjectStore::put is important for write-through caching
What changes are included in this PR?
Are there any user-facing changes?