drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From j..@apache.org
Subject drill git commit: DRILL-5391: CTAS: make folder and file permission configurable
Date Mon, 01 May 2017 17:24:11 GMT
Repository: drill
Updated Branches:
  refs/heads/master 3e8b01d5b -> 1e0a14cc9


DRILL-5391: CTAS: make folder and file permission configurable

close #820


Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/1e0a14cc
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/1e0a14cc
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/1e0a14cc

Branch: refs/heads/master
Commit: 1e0a14cc945bb3f6d7a2e47100eabf049a3a6be8
Parents: 3e8b01d
Author: Arina Ielchiieva <arina.yelchiyeva@gmail.com>
Authored: Wed Apr 26 13:27:19 2017 +0000
Committer: Jinfeng Ni <jni@apache.org>
Committed: Sun Apr 30 22:38:21 2017 -0700

----------------------------------------------------------------------
 .../org/apache/drill/exec/ExecConstants.java    |  3 +
 .../sql/handlers/CreateTableHandler.java        |  3 +-
 .../server/options/SystemOptionManager.java     |  3 +-
 .../apache/drill/exec/store/AbstractSchema.java |  2 +-
 .../drill/exec/store/StorageStrategy.java       | 69 ++++++++++-----
 .../exec/store/easy/json/JsonRecordWriter.java  |  2 +-
 .../exec/store/parquet/ParquetRecordWriter.java |  2 +-
 .../exec/store/text/DrillTextRecordWriter.java  |  2 +-
 .../org/apache/drill/exec/sql/TestCTAS.java     | 28 +++++-
 .../drill/exec/store/StorageStrategyTest.java   | 90 +++++++++++---------
 10 files changed, 138 insertions(+), 66 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java b/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
index 91498fc..e291524 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
@@ -446,4 +446,7 @@ public interface ExecConstants {
   String QUERY_TRANSIENT_STATE_UPDATE_KEY = "exec.query.progress.update";
   BooleanValidator QUERY_TRANSIENT_STATE_UPDATE = new BooleanValidator(QUERY_TRANSIENT_STATE_UPDATE_KEY,
true);
 
+  String PERSISTENT_TABLE_UMASK = "exec.persistent_table.umask";
+  StringValidator PERSISTENT_TABLE_UMASK_VALIDATOR = new StringValidator(PERSISTENT_TABLE_UMASK,
"002");
+
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java
index d50391c..72444ca 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/CreateTableHandler.java
@@ -91,7 +91,8 @@ public class CreateTableHandler extends DefaultSqlHandler {
     log("Calcite", newTblRelNodeWithPCol, logger, null);
     // Convert the query to Drill Logical plan and insert a writer operator on top.
     StorageStrategy storageStrategy = sqlCreateTable.isTemporary() ?
-        StorageStrategy.TEMPORARY : StorageStrategy.PERSISTENT;
+        StorageStrategy.TEMPORARY :
+        new StorageStrategy(context.getOption(ExecConstants.PERSISTENT_TABLE_UMASK).string_val,
false);
 
     // If we are creating temporary table, initial table name will be replaced with generated
table name.
     // Generated table name is unique, UUID.randomUUID() is used for its generation.

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
index a7d6526..8d0e96c 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
@@ -169,7 +169,8 @@ public class SystemOptionManager extends BaseOptionManager implements
OptionMana
       ExecConstants.ENABLE_QUERY_PROFILE_VALIDATOR,
       ExecConstants.QUERY_PROFILE_DEBUG_VALIDATOR,
       ExecConstants.USE_DYNAMIC_UDFS,
-      ExecConstants.QUERY_TRANSIENT_STATE_UPDATE
+      ExecConstants.QUERY_TRANSIENT_STATE_UPDATE,
+      ExecConstants.PERSISTENT_TABLE_UMASK_VALIDATOR
     };
     final Map<String, OptionValidator> tmp = new HashMap<>();
     for (final OptionValidator validator : validators) {

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractSchema.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractSchema.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractSchema.java
index 618841b..7d6bfe3 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractSchema.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractSchema.java
@@ -140,7 +140,7 @@ public abstract class AbstractSchema implements Schema, SchemaPartitionExplorer,
    * @return create table entry
    */
   public CreateTableEntry createNewTable(String tableName, List<String> partitionColumns)
{
-    return createNewTable(tableName, partitionColumns, StorageStrategy.PERSISTENT);
+    return createNewTable(tableName, partitionColumns, StorageStrategy.DEFAULT);
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/store/StorageStrategy.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/StorageStrategy.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/StorageStrategy.java
index a125bae..fdb8da8 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/StorageStrategy.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/StorageStrategy.java
@@ -18,6 +18,7 @@
 package org.apache.drill.exec.store;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.collect.Lists;
 import org.apache.hadoop.fs.FileSystem;
@@ -31,12 +32,11 @@ import java.util.List;
 public class StorageStrategy {
 
   /**
-   * Primary is used for persistent tables.
    * For directories: drwxrwxr-x (owner and group have full access, others can read and execute).
-   * For files: -rw-r--r-- (owner can read and write, group and others can read).
+   * For files: -rw-rw--r-- (owner and group can read and write, others can read).
    * Folders and files are not deleted on file system close.
    */
-  public static final StorageStrategy PERSISTENT = new StorageStrategy("775", "644", false);
+  public static final StorageStrategy DEFAULT = new StorageStrategy("002", false);
 
   /**
    * Primary is used for temporary tables.
@@ -44,32 +44,45 @@ public class StorageStrategy {
    * For files: -rw------- (owner can read and write, group and others have no access).
    * Folders and files are deleted on file system close.
    */
-  public static final StorageStrategy TEMPORARY = new StorageStrategy("700", "600", true);
+  public static final StorageStrategy TEMPORARY = new StorageStrategy("077", true);
 
-  private final String folderPermission;
-  private final String filePermission;
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(StorageStrategy.class);
+
+  private final String umask;
   private final boolean deleteOnExit;
 
   @JsonCreator
-  public StorageStrategy(@JsonProperty("folderPermission") String folderPermission,
-                         @JsonProperty("filePermission") String filePermission,
+  public StorageStrategy(@JsonProperty("umask") String umask,
                          @JsonProperty("deleteOnExit") boolean deleteOnExit) {
-    this.folderPermission = folderPermission;
-    this.filePermission = filePermission;
+    this.umask = validateUmask(umask);
     this.deleteOnExit = deleteOnExit;
   }
 
-  public String getFolderPermission() {
-    return folderPermission;
+  public String getUmask() {
+    return umask;
   }
 
-  public String getFilePermission() { return filePermission; }
-
   public boolean isDeleteOnExit() {
     return deleteOnExit;
   }
 
   /**
+   * @return folder permission after applying umask
+   */
+  @JsonIgnore
+  public FsPermission getFolderPermission() {
+    return FsPermission.getDirDefault().applyUMask(new FsPermission(umask));
+  }
+
+  /**
+   * @return file permission after applying umask
+   */
+  @JsonIgnore
+  public FsPermission getFilePermission() {
+    return FsPermission.getFileDefault().applyUMask(new FsPermission(umask));
+  }
+
+  /**
    * Creates passed path on appropriate file system.
    * Before creation checks which parent directories do not exists.
    * Applies storage strategy rules to all newly created directories.
@@ -94,7 +107,7 @@ public class StorageStrategy {
     }
     fs.mkdirs(path);
     for (Path location : locations) {
-      applyStrategy(fs, location, folderPermission, deleteOnExit);
+      applyStrategy(fs, location, getFolderPermission(), deleteOnExit);
     }
     return locations.get(locations.size() - 1);
   }
@@ -130,7 +143,7 @@ public class StorageStrategy {
     }
 
     for (Path location : locations) {
-      applyStrategy(fs, location, folderPermission, deleteOnExit);
+      applyStrategy(fs, location, getFolderPermission(), deleteOnExit);
     }
     return locations.get(locations.size() - 1);
   }
@@ -145,7 +158,25 @@ public class StorageStrategy {
    *         or adding file to delete on exit list
    */
   public void applyToFile(FileSystem fs, Path file) throws IOException {
-    applyStrategy(fs, file, filePermission, deleteOnExit);
+    applyStrategy(fs, file, getFilePermission(), deleteOnExit);
+  }
+
+  /**
+   * Validates if passed umask is valid.
+   * If umask is valid, returns given umask.
+   * If umask is invalid, returns default umask and logs error.
+   *
+   * @param umask umask string representation
+   * @return valid umask value
+   */
+  private String validateUmask(String umask) {
+    try {
+      new FsPermission(umask);
+      return umask;
+    } catch (IllegalArgumentException | NullPointerException e) {
+      logger.error("Invalid umask value [{}]. Using default [{}].", umask, DEFAULT.getUmask(),
e);
+      return DEFAULT.getUmask();
+    }
   }
 
   /**
@@ -185,8 +216,8 @@ public class StorageStrategy {
    * @throws IOException is thrown in case of problems while setting permission
    *         or adding path to delete on exit list
    */
-  private void applyStrategy(FileSystem fs, Path path, String permission, boolean deleteOnExit)
throws IOException {
-    fs.setPermission(path, new FsPermission(permission));
+  private void applyStrategy(FileSystem fs, Path path, FsPermission permission, boolean deleteOnExit)
throws IOException {
+    fs.setPermission(path, permission);
     if (deleteOnExit) {
       fs.deleteOnExit(path);
     }

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java
index c37da8a..345c056 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonRecordWriter.java
@@ -65,7 +65,7 @@ public class JsonRecordWriter extends JSONOutputRecordWriter implements
RecordWr
   private boolean fRecordStarted = false; // true once the startRecord() is called until
endRecord() is called
 
   public JsonRecordWriter(StorageStrategy storageStrategy){
-    this.storageStrategy = storageStrategy == null ? StorageStrategy.PERSISTENT : storageStrategy;
+    this.storageStrategy = storageStrategy == null ? StorageStrategy.DEFAULT : storageStrategy;
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java
index 6850d36..7536d78 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetRecordWriter.java
@@ -121,7 +121,7 @@ public class ParquetRecordWriter extends ParquetOutputRecordWriter {
     this.hasPartitions = partitionColumns != null && partitionColumns.size() >
0;
     this.extraMetaData.put(DRILL_VERSION_PROPERTY, DrillVersionInfo.getVersion());
     this.extraMetaData.put(WRITER_VERSION_PROPERTY, String.valueOf(ParquetWriter.WRITER_VERSION));
-    this.storageStrategy = writer.getStorageStrategy() == null ? StorageStrategy.PERSISTENT
: writer.getStorageStrategy();
+    this.storageStrategy = writer.getStorageStrategy() == null ? StorageStrategy.DEFAULT
: writer.getStorageStrategy();
     this.cleanUpLocations = Lists.newArrayList();
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java
index d65a3eb..f991abb 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/text/DrillTextRecordWriter.java
@@ -58,7 +58,7 @@ public class DrillTextRecordWriter extends StringOutputRecordWriter {
 
   public DrillTextRecordWriter(BufferAllocator allocator, StorageStrategy storageStrategy)
{
     super(allocator);
-    this.storageStrategy = storageStrategy == null ? StorageStrategy.PERSISTENT : storageStrategy;
+    this.storageStrategy = storageStrategy == null ? StorageStrategy.DEFAULT : storageStrategy;
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTAS.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTAS.java b/exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTAS.java
index 9d9b403..88d23d3 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTAS.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestCTAS.java
@@ -1,4 +1,4 @@
-/**
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -20,13 +20,20 @@ package org.apache.drill.exec.sql;
 import com.google.common.collect.Maps;
 import org.apache.commons.io.FileUtils;
 import org.apache.drill.BaseTestQuery;
+import org.apache.drill.exec.ExecConstants;
 import org.apache.drill.exec.proto.UserBitShared;
 import org.apache.drill.exec.rpc.user.QueryDataBatch;
+import org.apache.drill.exec.store.StorageStrategy;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
 import org.junit.Test;
 
 import java.io.File;
 import java.util.Map;
 
+import static org.junit.Assert.assertEquals;
+
 public class TestCTAS extends BaseTestQuery {
   @Test // DRILL-2589
   public void withDuplicateColumnsInDef1() throws Exception {
@@ -276,6 +283,25 @@ public class TestCTAS extends BaseTestQuery {
     }
   }
 
+  @Test
+  public void createTableWithCustomUmask() throws Exception {
+    test("use %s", TEMP_SCHEMA);
+    String tableName = "with_custom_permission";
+    StorageStrategy storageStrategy = new StorageStrategy("000", false);
+    try (FileSystem fs = FileSystem.get(new Configuration())) {
+      test("alter session set `%s` = '%s'", ExecConstants.PERSISTENT_TABLE_UMASK, storageStrategy.getUmask());
+      test("create table %s as select 'A' from (values(1))", tableName);
+      Path tableLocation = new Path(getDfsTestTmpSchemaLocation(), tableName);
+      assertEquals("Directory permission should match",
+          storageStrategy.getFolderPermission(), fs.getFileStatus(tableLocation).getPermission());
+      assertEquals("File permission should match",
+          storageStrategy.getFilePermission(), fs.listLocatedStatus(tableLocation).next().getPermission());
+    } finally {
+      test("alter session reset `%s`", ExecConstants.PERSISTENT_TABLE_UMASK);
+      test("drop table if exists %s", tableName);
+    }
+  }
+
   private static void ctasErrorTestHelper(final String ctasSql, final String expErrorMsg)
throws Exception {
     final String createTableSql = String.format(ctasSql, TEMP_SCHEMA, "testTableName");
     errorMsgTestHelper(createTableSql, expErrorMsg);

http://git-wip-us.apache.org/repos/asf/drill/blob/1e0a14cc/exec/java-exec/src/test/java/org/apache/drill/exec/store/StorageStrategyTest.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/store/StorageStrategyTest.java
b/exec/java-exec/src/test/java/org/apache/drill/exec/store/StorageStrategyTest.java
index 6a377ec..32eb234 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/exec/store/StorageStrategyTest.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/store/StorageStrategyTest.java
@@ -16,6 +16,7 @@
  */
 package org.apache.drill.exec.store;
 
+import com.google.common.collect.Lists;
 import com.google.common.io.Files;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileSystem;
@@ -33,11 +34,11 @@ import static org.junit.Assert.assertTrue;
 
 public class StorageStrategyTest {
 
-  private static final Configuration configuration = new Configuration();
-  private static final FsPermission full_permission = new FsPermission("777");
-  private static final StorageStrategy persistent_strategy = new StorageStrategy("775", "644",
false);
-  private static final StorageStrategy temporary_strategy = new StorageStrategy("700", "600",
true);
-  private FileSystem fs;
+  private static final Configuration CONFIGURATION = new Configuration();
+  private static final FsPermission FULL_PERMISSION = FsPermission.getDirDefault();
+  private static final StorageStrategy PERSISTENT_STRATEGY = new StorageStrategy("002", false);
+  private static final StorageStrategy TEMPORARY_STRATEGY = new StorageStrategy("077", true);
+  private FileSystem FS;
 
   @Before
   public void setup() throws Exception {
@@ -50,10 +51,10 @@ public class StorageStrategyTest {
     Path file = addNLevelsAndFile(initialPath, 2, true);
     Path firstCreatedParentPath = addNLevelsAndFile(initialPath, 1, false);
 
-    Path createdParentPath = persistent_strategy.createFileAndApply(fs, file);
+    Path createdParentPath = PERSISTENT_STRATEGY.createFileAndApply(FS, file);
 
     assertEquals("Path should match", firstCreatedParentPath, createdParentPath);
-    checkPathAndPermission(initialPath, file, true, 2, persistent_strategy);
+    checkPathAndPermission(initialPath, file, true, 2, PERSISTENT_STRATEGY);
     checkDeleteOnExit(firstCreatedParentPath, true);
   }
 
@@ -63,10 +64,10 @@ public class StorageStrategyTest {
     Path file = addNLevelsAndFile(initialPath, 2, true);
     Path firstCreatedParentPath = addNLevelsAndFile(initialPath, 1, false);
 
-    Path createdParentPath = temporary_strategy.createFileAndApply(fs, file);
+    Path createdParentPath = TEMPORARY_STRATEGY.createFileAndApply(FS, file);
 
     assertEquals("Path should match", firstCreatedParentPath, createdParentPath);
-    checkPathAndPermission(initialPath, file, true, 2, temporary_strategy);
+    checkPathAndPermission(initialPath, file, true, 2, TEMPORARY_STRATEGY);
     checkDeleteOnExit(firstCreatedParentPath, false);
   }
 
@@ -75,10 +76,10 @@ public class StorageStrategyTest {
     Path initialPath = prepareStorageDirectory();
     Path file = addNLevelsAndFile(initialPath, 0, true);
 
-    Path createdFile = persistent_strategy.createFileAndApply(fs, file);
+    Path createdFile = PERSISTENT_STRATEGY.createFileAndApply(FS, file);
 
     assertEquals("Path should match", file, createdFile);
-    checkPathAndPermission(initialPath, file, true, 0, persistent_strategy);
+    checkPathAndPermission(initialPath, file, true, 0, PERSISTENT_STRATEGY);
     checkDeleteOnExit(file, true);
   }
 
@@ -87,10 +88,10 @@ public class StorageStrategyTest {
     Path initialPath = prepareStorageDirectory();
     Path file = addNLevelsAndFile(initialPath, 0, true);
 
-    Path createdFile = temporary_strategy.createFileAndApply(fs, file);
+    Path createdFile = TEMPORARY_STRATEGY.createFileAndApply(FS, file);
 
     assertEquals("Path should match", file, createdFile);
-    checkPathAndPermission(initialPath, file, true, 0, temporary_strategy);
+    checkPathAndPermission(initialPath, file, true, 0, TEMPORARY_STRATEGY);
     checkDeleteOnExit(file, false);
   }
 
@@ -98,13 +99,13 @@ public class StorageStrategyTest {
   public void testFailureOnExistentFile() throws Exception {
     Path initialPath = prepareStorageDirectory();
     Path file = addNLevelsAndFile(initialPath, 0, true);
-    fs.createNewFile(file);
-    assertTrue("File should exist", fs.exists(file));
+    FS.createNewFile(file);
+    assertTrue("File should exist", FS.exists(file));
     try {
-      persistent_strategy.createFileAndApply(fs, file);
+      PERSISTENT_STRATEGY.createFileAndApply(FS, file);
     } catch (IOException e) {
       assertEquals("Error message should match", String.format("File [%s] already exists
on file system [%s].",
-          file.toUri().getPath(), fs.getUri()), e.getMessage());
+          file.toUri().getPath(), FS.getUri()), e.getMessage());
       throw e;
     }
   }
@@ -115,10 +116,10 @@ public class StorageStrategyTest {
     Path resultPath = addNLevelsAndFile(initialPath, 2, false);
     Path firstCreatedParentPath = addNLevelsAndFile(initialPath, 1, false);
 
-    Path createdParentPath = persistent_strategy.createPathAndApply(fs, resultPath);
+    Path createdParentPath = PERSISTENT_STRATEGY.createPathAndApply(FS, resultPath);
 
     assertEquals("Path should match", firstCreatedParentPath, createdParentPath);
-    checkPathAndPermission(initialPath, resultPath, false, 2, persistent_strategy);
+    checkPathAndPermission(initialPath, resultPath, false, 2, PERSISTENT_STRATEGY);
     checkDeleteOnExit(firstCreatedParentPath, true);
   }
 
@@ -128,10 +129,10 @@ public class StorageStrategyTest {
     Path resultPath = addNLevelsAndFile(initialPath, 2, false);
     Path firstCreatedParentPath = addNLevelsAndFile(initialPath, 1, false);
 
-    Path createdParentPath = temporary_strategy.createPathAndApply(fs, resultPath);
+    Path createdParentPath = TEMPORARY_STRATEGY.createPathAndApply(FS, resultPath);
 
     assertEquals("Path should match", firstCreatedParentPath, createdParentPath);
-    checkPathAndPermission(initialPath, resultPath, false, 2, temporary_strategy);
+    checkPathAndPermission(initialPath, resultPath, false, 2, TEMPORARY_STRATEGY);
     checkDeleteOnExit(firstCreatedParentPath, false);
   }
 
@@ -139,46 +140,55 @@ public class StorageStrategyTest {
   public void testCreateNoPath() throws Exception {
     Path path = prepareStorageDirectory();
 
-    Path createdParentPath = temporary_strategy.createPathAndApply(fs, path);
+    Path createdParentPath = TEMPORARY_STRATEGY.createPathAndApply(FS, path);
 
     assertNull("Path should be null", createdParentPath);
-    assertEquals("Permission should match", full_permission, fs.getFileStatus(path).getPermission());
+    assertEquals("Permission should match", FULL_PERMISSION, FS.getFileStatus(path).getPermission());
   }
 
   @Test
   public void testStrategyForExistingFile() throws Exception {
     Path initialPath = prepareStorageDirectory();
     Path file = addNLevelsAndFile(initialPath, 0, true);
-    fs.createNewFile(file);
-    fs.setPermission(file, full_permission);
+    FS.createNewFile(file);
+    FS.setPermission(file, FULL_PERMISSION);
 
-    assertTrue("File should exist", fs.exists(file));
-    assertEquals("Permission should match", full_permission, fs.getFileStatus(file).getPermission());
+    assertTrue("File should exist", FS.exists(file));
+    assertEquals("Permission should match", FULL_PERMISSION, FS.getFileStatus(file).getPermission());
 
-    temporary_strategy.applyToFile(fs, file);
+    TEMPORARY_STRATEGY.applyToFile(FS, file);
 
-    assertEquals("Permission should match", new FsPermission(temporary_strategy.getFilePermission()),
-        fs.getFileStatus(file).getPermission());
+    assertEquals("Permission should match", new FsPermission(TEMPORARY_STRATEGY.getFilePermission()),
+        FS.getFileStatus(file).getPermission());
     checkDeleteOnExit(file, false);
   }
 
+  @Test
+  public void testInvalidUmask() throws Exception {
+    for (String invalid : Lists.newArrayList("ABC", "999", null)) {
+      StorageStrategy storageStrategy = new StorageStrategy(invalid, true);
+      assertEquals("Umask value should match default", StorageStrategy.DEFAULT.getUmask(),
storageStrategy.getUmask());
+      assertTrue("deleteOnExit flag should be set to true", storageStrategy.isDeleteOnExit());
+    }
+  }
+
   private Path prepareStorageDirectory() throws IOException {
     File storageDirectory = Files.createTempDir();
     storageDirectory.deleteOnExit();
     Path path = new Path(storageDirectory.toURI().getPath());
-    fs.setPermission(path, full_permission);
+    FS.setPermission(path, FULL_PERMISSION);
     return path;
   }
 
   private void initFileSystem() throws IOException {
-    if (fs != null) {
+    if (FS != null) {
       try {
-        fs.close();
+        FS.close();
       } catch (Exception e) {
         // do nothing
       }
     }
-    fs = FileSystem.get(configuration);
+    FS = FileSystem.get(CONFIGURATION);
   }
 
   private Path addNLevelsAndFile(Path initialPath, int levels, boolean addFile) {
@@ -198,25 +208,25 @@ public class StorageStrategyTest {
                                       int levels,
                                       StorageStrategy storageStrategy) throws IOException
{
 
-    assertEquals("Path type should match", isFile, fs.isFile(resultPath));
-    assertEquals("Permission should match", full_permission, fs.getFileStatus(initialPath).getPermission());
+    assertEquals("Path type should match", isFile, FS.isFile(resultPath));
+    assertEquals("Permission should match", FULL_PERMISSION, FS.getFileStatus(initialPath).getPermission());
 
     if (isFile) {
       assertEquals("Permission should match", new FsPermission(storageStrategy.getFilePermission()),
-          fs.getFileStatus(resultPath).getPermission());
+          FS.getFileStatus(resultPath).getPermission());
     }
     Path startingPath = initialPath;
     FsPermission folderPermission = new FsPermission(storageStrategy.getFolderPermission());
     for (int i = 1; i <= levels; i++) {
       startingPath = new Path(startingPath, "level" + i);
-      assertEquals("Permission should match", folderPermission, fs.getFileStatus(startingPath).getPermission());
+      assertEquals("Permission should match", folderPermission, FS.getFileStatus(startingPath).getPermission());
     }
   }
 
   private void checkDeleteOnExit(Path path, boolean isPresent) throws IOException {
-    assertTrue("Path should be present", fs.exists(path));
+    assertTrue("Path should be present", FS.exists(path));
     // close and open file system to check for path presence
     initFileSystem();
-    assertEquals("Path existence flag should match", isPresent, fs.exists(path));
+    assertEquals("Path existence flag should match", isPresent, FS.exists(path));
   }
 }


Mime
View raw message