alamb commented on code in PR #3781:
URL: https://github.com/apache/arrow-rs/pull/3781#discussion_r1123243987


##########
object_store/src/lib.rs:
##########
@@ -840,6 +840,12 @@ mod tests {
             crate::Error::NotFound { .. }
         ));
 
+        let files = flatten_list_stream(storage, None).await.unwrap();

Review Comment:
   I didn't see a test for atomic `puts` (this appears to be testing atomic 
`put_multipart`). 
   
   Maybe I am misreading the code, but I think there should be a separate test 
to ensure we don't accidentally break `put` in the future (and that the other 
implementations are also atomic)



##########
object_store/src/local.rs:
##########
@@ -282,28 +290,10 @@ impl ObjectStore for LocalFileSystem {
     ) -> Result<(MultipartId, Box<dyn AsyncWrite + Unpin + Send>)> {
         let dest = self.config.path_to_filesystem(location)?;
 
-        // Generate an id in case of concurrent writes
-        let mut multipart_id = 1;
-
-        // Will write to a temporary path
-        let staging_path = loop {
-            let staging_path = get_upload_stage_path(&dest, 
&multipart_id.to_string());
-
-            match std::fs::metadata(&staging_path) {

Review Comment:
   TIL: https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use



-- 
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]

Reply via email to