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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
