[GitHub] [hadoop] steveloughran commented on a change in pull request #4036: HADOOP-18145.Decompress the ZIP file and retain the original file per…

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

steveloughran commented on a change in pull request #4036:
URL: https://github.com/apache/hadoop/pull/4036#discussion_r815939653



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##########
@@ -36,8 +36,11 @@
 import java.nio.charset.CharsetEncoder;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.AccessDeniedException;
+import java.nio.file.attribute.PosixFilePermission;
 import java.nio.file.FileSystems;
 import java.nio.file.Files;
+
+import java.util.*;

Review comment:
       no .* expansions, just add any new imports into the existing ones. and tell your IDE to stop combining imports into .* for future hadoop prs. please: imports are backport pain

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##########
@@ -667,6 +670,9 @@ public static void unZip(InputStream inputStream, File toDir)
           if (!file.setLastModified(entry.getTime())) {
             numOfFailedLastModifiedSet++;
           }
+          if (entry.getPlatform() == ZipArchiveEntry.PLATFORM_UNIX) {
+            Files.setPosixFilePermissions(file.toPath(), permissionsFromMode(entry.getUnixMode()));

Review comment:
       what if this fails? 

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##########
@@ -726,7 +727,10 @@ public void testUnZip() throws IOException {
     // check result:
     assertTrue(new File(tmp, "foo").exists());
     assertEquals(12, new File(tmp, "foo").length());
-    
+    assertTrue(new File(tmp, "foo").canExecute());

Review comment:
       asserttrue tests need to say what is wrong when the assert fails. the goal is to make a failed yetus/test run meaningful.

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##########
@@ -726,7 +727,10 @@ public void testUnZip() throws IOException {
     // check result:
     assertTrue(new File(tmp, "foo").exists());
     assertEquals(12, new File(tmp, "foo").length());
-    
+    assertTrue(new File(tmp, "foo").canExecute());
+    assertTrue(new File(tmp, "foo").canRead());
+    assertTrue(new File(tmp, "foo").canWrite());

Review comment:
       1. there's no tests to verify that other/group permissions are picked up. not sure if it is possible.
   2. what happens if someone runs the test on windows?

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##########
@@ -676,6 +682,31 @@ public static void unZip(InputStream inputStream, File toDir)
     }
   }
 
+  public static Set<PosixFilePermission> permissionsFromMode(Integer mode) {

Review comment:
       is mode ever null?

##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java
##########
@@ -715,6 +746,9 @@ public static void unZip(File inFile, File unzipDir) throws IOException {
               }
             } finally {
               out.close();
+              if (entry.getPlatform() == ZipArchiveEntry.PLATFORM_UNIX) {

Review comment:
       this is going to execute even if writing to out failed. not sure that is ideal.
   better to put it on a line after the try/finally block. so is only called if the write succeeded.




-- 
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: common-issues-unsubscribe@hadoop.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org