Xuanwo commented on code in PR #5562: URL: https://github.com/apache/opendal/pull/5562#discussion_r1922516914
########## core/src/raw/adapters/kv/backend.rs: ########## @@ -297,9 +297,11 @@ impl<S: Adapter> oio::Write for KvWriter<S> { Ok(()) } - async fn close(&mut self) -> Result<()> { + async fn close(&mut self) -> Result<Metadata> { let buf = self.buffer.clone().collect(); - self.kv.set(&self.path, buf).await + self.kv.set(&self.path, buf).await?; + + Ok(Metadata::default()) Review Comment: Maybe we can return at least the content length of file? ########## core/src/raw/oio/write/append_write.rs: ########## @@ -96,8 +96,8 @@ where Ok(()) } - async fn close(&mut self) -> Result<()> { - Ok(()) + async fn close(&mut self) -> Result<Metadata> { + Ok(Metadata::default()) Review Comment: Perhaps we should return valid data here. The metadata could be returned and replaced by `self.inner.append().await`, then stored within `AppendWriter` until the user calls `close`. ########## core/tests/behavior/async_write.rs: ########## @@ -551,6 +588,55 @@ pub async fn test_writer_futures_copy_with_concurrent(op: Operator) -> Result<() Ok(()) } +pub async fn test_writer_return_metadata(op: Operator) -> Result<()> { + if !op.info().full_capability().write_can_multi { + return Ok(()); + } + + let path = TEST_FIXTURE.new_file_path(); + let size = 5 * 1024 * 1024; // write file with 5 MiB + let content_a = gen_fixed_bytes(size); + let content_b = gen_fixed_bytes(size); + + let mut w = op.writer(&path).await?; + w.write(content_a.clone()).await?; + w.write(content_b.clone()).await?; + let meta = w.close().await?; + + let stat_meta = op.stat(&path).await.expect("stat must succeed"); + + if meta.content_length() != 0 { + assert_eq!(stat_meta.content_length(), meta.content_length()); + } + if let Some(last_modified_time) = meta.last_modified() { + assert_eq!( Review Comment: Maybe it's more readable to use the following? ```rust assert_eq!(meta.last_modified(), stat_meta.last_modified()) ``` ########## core/src/services/s3/writer.rs: ########## @@ -159,18 +181,25 @@ impl oio::MultipartWrite for S3Writer { let status = resp.status(); + let mut meta = S3Writer::parse_header_into_meta(&self.path, resp.headers())?; + match status { StatusCode::OK => { // still check if there is any error because S3 might return error for status code 200 // https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html#API_CompleteMultipartUpload_Example_4 let (parts, body) = resp.into_parts(); - let maybe_error: S3Error = - quick_xml::de::from_reader(body.reader()).map_err(new_xml_deserialize_error)?; + + let maybe_error: S3Error = quick_xml::de::from_reader(body.clone().reader()) + .map_err(new_xml_deserialize_error)?; if !maybe_error.code.is_empty() { return Err(from_s3_error(maybe_error, parts)); } - Ok(()) + let ret: CompleteMultipartUploadResult = Review Comment: Maybe we can embed the error struct in `CompleteMultipartUploadResult` so that we don't need to parse twice. ########## core/src/services/aliyun_drive/writer.rs: ########## @@ -108,13 +108,13 @@ impl oio::Write for AliyunDriveWriter { Ok(()) } - async fn close(&mut self) -> Result<()> { + async fn close(&mut self) -> Result<Metadata> { let (Some(upload_id), Some(file_id)) = (self.upload_id.as_ref(), self.file_id.as_ref()) else { - return Ok(()); + return Ok(Metadata::default()); Review Comment: I believe we need to set a content length for each service, at the very least. `content_length` is specifically handled in OpenDAL, where users will receive `0` if it doesn't set. ########## core/src/raw/adapters/typed_kv/backend.rs: ########## @@ -301,7 +301,8 @@ impl<S: Adapter> oio::Write for KvWriter<S> { } }; self.kv.set(&self.path, value).await?; - Ok(()) + + Ok(Metadata::default()) Review Comment: We can return back the metadata in value. ########## core/tests/behavior/async_write.rs: ########## @@ -551,6 +588,55 @@ pub async fn test_writer_futures_copy_with_concurrent(op: Operator) -> Result<() Ok(()) } +pub async fn test_writer_return_metadata(op: Operator) -> Result<()> { + if !op.info().full_capability().write_can_multi { + return Ok(()); + } + + let path = TEST_FIXTURE.new_file_path(); + let size = 5 * 1024 * 1024; // write file with 5 MiB + let content_a = gen_fixed_bytes(size); + let content_b = gen_fixed_bytes(size); + + let mut w = op.writer(&path).await?; + w.write(content_a.clone()).await?; + w.write(content_b.clone()).await?; + let meta = w.close().await?; + + let stat_meta = op.stat(&path).await.expect("stat must succeed"); + + if meta.content_length() != 0 { + assert_eq!(stat_meta.content_length(), meta.content_length()); + } + if let Some(last_modified_time) = meta.last_modified() { + assert_eq!( + stat_meta + .last_modified() + .expect("last modified time mut exist"), + last_modified_time + ); + } + if let Some(etag) = meta.etag() { Review Comment: I prefer to comment this test until rados fix it. ########## core/src/raw/adapters/kv/backend.rs: ########## @@ -314,10 +316,10 @@ impl<S: Adapter> oio::BlockingWrite for KvWriter<S> { Ok(()) } - fn close(&mut self) -> Result<()> { + fn close(&mut self) -> Result<Metadata> { let buf = self.buffer.clone().collect(); self.kv.blocking_set(&self.path, buf)?; - Ok(()) + Ok(Metadata::default()) Review Comment: The same. ########## core/src/raw/adapters/typed_kv/backend.rs: ########## @@ -330,7 +331,7 @@ impl<S: Adapter> oio::BlockingWrite for KvWriter<S> { }; kv.blocking_set(&self.path, value)?; - Ok(()) + Ok(Metadata::default()) Review Comment: The same. -- 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: commits-unsubscr...@opendal.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org