[GitHub] [arrow-rs] alamb commented on a diff in pull request #3781: Make LocalFileSystem::put atomic (#3780)

["alamb (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org