alamb commented on code in PR #4944:
URL: https://github.com/apache/arrow-rs/pull/4944#discussion_r1362726955
##########
object_store/src/aws/client.rs:
##########
@@ -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>(
Review Comment:
this structure (`S3Client`) is not public, so this is not a breaking API
change in case anyone else was wondering
##########
object_store/src/lib.rs:
##########
@@ -850,6 +850,13 @@ impl GetResult {
}
}
+/// Result for a put request
+#[derive(Debug)]
+pub struct PutResult {
+ /// The unique identifier for the object
Review 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/azure/mod.rs:
##########
@@ -170,11 +170,13 @@ impl std::fmt::Display for MicrosoftAzure {
#[async_trait]
impl ObjectStore for MicrosoftAzure {
- async fn put(&self, location: &Path, bytes: Bytes) -> Result<()> {
- self.client
+ async fn put(&self, location: &Path, bytes: Bytes) -> Result<PutResult> {
+ let response = self
+ .client
.put_request(location, Some(bytes), false, &())
.await?;
- Ok(())
+ let e_tag = Some(get_etag(response.headers()).context(MetadataSnafu)?);
Review 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})` 🤔
##########
object_store/src/azure/mod.rs:
##########
@@ -170,11 +170,13 @@ impl std::fmt::Display for MicrosoftAzure {
#[async_trait]
impl ObjectStore for MicrosoftAzure {
- async fn put(&self, location: &Path, bytes: Bytes) -> Result<()> {
- self.client
+ async fn put(&self, location: &Path, bytes: Bytes) -> Result<PutResult> {
+ let response = self
+ .client
.put_request(location, Some(bytes), false, &())
.await?;
- Ok(())
+ let e_tag = Some(get_etag(response.headers()).context(MetadataSnafu)?);
Review 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})` 🤔
##########
object_store/src/memory.rs:
##########
@@ -122,9 +124,11 @@ impl std::fmt::Display for InMemory {
#[async_trait]
impl ObjectStore for InMemory {
- async fn put(&self, location: &Path, bytes: Bytes) -> Result<()> {
- self.storage.write().insert(location, bytes);
- Ok(())
+ async fn put(&self, location: &Path, bytes: Bytes) -> Result<PutResult> {
+ let etag = self.storage.write().insert(location, bytes);
+ Ok(PutResult {
+ e_tag: Some(etag.to_string()),
+ })
Review Comment:
If you made PutResult have non pub fields, maybe this could be a builder
style API like this
```rust
OK(PutResult::new().with_etag(etag))
```
##########
object_store/src/lib.rs:
##########
@@ -850,6 +850,13 @@ impl GetResult {
}
}
+/// Result for a put request
+#[derive(Debug)]
Review Comment:
I always find it annoying when these traits are not derived for simple
objects.
```suggestion
#[derive(Debug, Hash, Clone, PartialOrd)]
```
##########
object_store/src/aws/mod.rs:
##########
@@ -365,10 +359,9 @@ struct S3MultiPartUpload {
#[async_trait]
impl PutPart for S3MultiPartUpload {
async fn put_part(&self, buf: Vec<u8>, part_idx: usize) -> Result<PartId> {
- use reqwest::header::ETAG;
let part = (part_idx + 1).to_string();
- let response = self
+ let content_id = self
Review Comment:
is there a reason this is called `content_id` here and `etag` elsewhere? I
just found the difference in terminology confusing
##########
object_store/src/lib.rs:
##########
@@ -850,6 +850,13 @@ impl GetResult {
}
}
+/// Result for a put request
+#[derive(Debug)]
+pub struct PutResult {
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
