From 47997e92b6cd525f35552380f527b9fa3e8ae466 Mon Sep 17 00:00:00 2001
From: Ayush Saxena <ayushsaxena@apache.org>
Date: Fri, 6 Sep 2024 18:07:57 +0530
Subject: [PATCH] TEZ-4413: Fix permission may not be set after crash. (#368).
 (Ayush Saxena, reviewed by Laszlo Bodor)

---
 .../logging/proto/DatePartitionedLogger.java  | 13 +++++++++++-
 .../proto/TestProtoHistoryLoggingService.java | 20 +++++++++++++++++++
 2 files changed, 32 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..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
@@ -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,20 @@ public DatePartitionedLogger(Parser<T> 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: 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) {
         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<DAGHistoryEvent> makeHistoryEvents(TezDAGID dagId,
       ProtoHistoryLoggingService service) {
     List<DAGHistoryEvent> historyEvents = new ArrayList<>();