[GitHub] gianm commented on a change in pull request #6677: FileUtils: Sync directory entry too on writeAtomically.

[GitBox <gi...@apache.org>]

gianm commented on a change in pull request #6677: FileUtils: Sync directory entry too on writeAtomically.
URL: https://github.com/apache/incubator-druid/pull/6677#discussion_r237169153
 
 

 ##########
 File path: core/src/main/java/org/apache/druid/java/util/common/CompressionUtils.java
 ##########
 @@ -78,16 +78,12 @@ public static long zip(File directory, File outputZipFile, boolean fsync) throws
       log.warn("No .zip suffix[%s], putting files from [%s] into it anyway.", outputZipFile, directory);
     }
 
-    try (final FileOutputStream out = new FileOutputStream(outputZipFile)) {
-      long bytes = zip(directory, out);
-
-      // For explanation of why fsyncing here is a good practice:
-      // https://github.com/apache/incubator-druid/pull/5187#pullrequestreview-85188984
-      if (fsync) {
-        out.getChannel().force(true);
+    if (fsync) {
+      return FileUtils.writeAtomically(outputZipFile, out -> zip(directory, out));
+    } else {
+      try (final FileOutputStream out = new FileOutputStream(outputZipFile)) {
 
 Review comment:
   Oh, interesting, I didn't know that. I'll change it.
   
   Do you have a sense of roughly how expensive a finalizer is? Like is it bad enough that we should care a lot about adjusting old code, or not so bad, and we should only care for adjusting old code that creates a lot of objects (maybe code that creates lots of short lived tmp files).
   
   From looking around online it seems that the JDK will probably remove finalization from these classes eventually (https://bugs.openjdk.java.net/browse/JDK-8187325).

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org