From 8b2b2863814b92ca5726e031a46e44936c437a5b Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Thu, 29 Aug 2024 10:29:02 +0530 Subject: [PATCH 1/2] TEZ-4413: Fix permission may not be set after crash. --- .../logging/proto/DatePartitionedLogger.java | 12 ++++++++++- .../proto/TestProtoHistoryLoggingService.java | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java b/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java index a569567d1e..69c1c41151 100644 --- a/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java +++ b/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java @@ -18,6 +18,7 @@ package org.apache.tez.dag.history.logging.proto; +import java.io.FileNotFoundException; import java.io.IOException; import java.time.LocalDate; import java.time.LocalDateTime; @@ -71,10 +72,19 @@ public DatePartitionedLogger(Parser parser, Path baseDir, Configuration conf, private void createDirIfNotExists(Path path) throws IOException { FileSystem fileSystem = path.getFileSystem(conf); + FileStatus fileStatus = null; try { - if (!fileSystem.exists(path)) { + fileStatus = fileSystem.getFileStatus(path); + } catch (FileNotFoundException fnf) { + // ignore + } + try { + if (fileStatus == null) { fileSystem.mkdirs(path); fileSystem.setPermission(path, DIR_PERMISSION); + } else if (!fileStatus.getPermission().equals(DIR_PERMISSION)) { + LOG.info("Permission on path {} is {}, setting it to {}", path, fileStatus.getPermission(), DIR_PERMISSION); + fileSystem.setPermission(path, DIR_PERMISSION); } } catch (IOException e) { // Ignore this exception, if there is a problem it'll fail when trying to read or write. diff --git a/tez-plugins/tez-protobuf-history-plugin/src/test/java/org/apache/tez/dag/history/logging/proto/TestProtoHistoryLoggingService.java b/tez-plugins/tez-protobuf-history-plugin/src/test/java/org/apache/tez/dag/history/logging/proto/TestProtoHistoryLoggingService.java index 4f24d30a88..f599e61d2c 100644 --- a/tez-plugins/tez-protobuf-history-plugin/src/test/java/org/apache/tez/dag/history/logging/proto/TestProtoHistoryLoggingService.java +++ b/tez-plugins/tez-protobuf-history-plugin/src/test/java/org/apache/tez/dag/history/logging/proto/TestProtoHistoryLoggingService.java @@ -32,7 +32,10 @@ import com.google.protobuf.CodedInputStream; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.util.Time; import org.apache.hadoop.yarn.api.records.ApplicationAttemptId; import org.apache.hadoop.yarn.api.records.ApplicationId; import org.apache.hadoop.yarn.api.records.ContainerId; @@ -267,6 +270,23 @@ public void testServiceSplitEvents() throws Exception { scanner.close(); } + @Test + public void testDirPermissions() throws IOException { + Path basePath = new Path(tempFolder.newFolder().getAbsolutePath()); + Configuration conf = new Configuration(); + FileSystem fs = basePath.getFileSystem(conf); + FsPermission expectedPermissions = FsPermission.createImmutable((short) 01777); + + // Check the directory already exists and doesn't have the expected permissions. + Assert.assertTrue(fs.exists(basePath)); + Assert.assertNotEquals(expectedPermissions, fs.getFileStatus(basePath).getPermission()); + + new DatePartitionedLogger<>(HistoryEventProto.PARSER, basePath, conf, new FixedClock(Time.now())); + + // Check the permissions they should be same as the expected permissions + Assert.assertEquals(expectedPermissions, fs.getFileStatus(basePath).getPermission()); + } + private List makeHistoryEvents(TezDAGID dagId, ProtoHistoryLoggingService service) { List historyEvents = new ArrayList<>(); From 77243350e0401f8fa308d92eec25f9fbfd557824 Mon Sep 17 00:00:00 2001 From: Ayush Saxena Date: Fri, 6 Sep 2024 17:18:18 +0530 Subject: [PATCH 2/2] Update the comment. --- .../tez/dag/history/logging/proto/DatePartitionedLogger.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java b/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java index 69c1c41151..ee838646fb 100644 --- a/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java +++ b/tez-plugins/tez-protobuf-history-plugin/src/main/java/org/apache/tez/dag/history/logging/proto/DatePartitionedLogger.java @@ -76,7 +76,8 @@ private void createDirIfNotExists(Path path) throws IOException { try { fileStatus = fileSystem.getFileStatus(path); } catch (FileNotFoundException fnf) { - // ignore + // ignore: regardless of the outcome of FileSystem.getFileStatus call (exception or returning null), + // we handle the fileStatus == null case later on, so it's safe to ignore this exception now } try { if (fileStatus == null) {