drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] Ben-Zvi closed pull request #1296: DRILL-5365: Enforce Immutability of DrillFileSystem
Date Sun, 09 Sep 2018 05:05:43 GMT
Ben-Zvi closed pull request #1296: DRILL-5365: Enforce Immutability of DrillFileSystem
URL: https://github.com/apache/drill/pull/1296
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
index df78800a8dc..0edf974e16b 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
@@ -161,7 +161,7 @@ public ExternalSortBatch(ExternalSort popConfig, FragmentContext context,
Record
     this.incoming = incoming;
     DrillConfig config = context.getConfig();
     Configuration conf = new Configuration();
-    conf.set("fs.default.name", config.getString(ExecConstants.EXTERNAL_SORT_SPILL_FILESYSTEM));
+    conf.set(FileSystem.FS_DEFAULT_NAME_KEY, config.getString(ExecConstants.EXTERNAL_SORT_SPILL_FILESYSTEM));
     try {
       this.fs = FileSystem.get(conf);
     } catch (IOException e) {
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
index b230d7febbc..d09531879e1 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
@@ -36,7 +36,6 @@
 import org.apache.hadoop.fs.CreateFlag;
 import org.apache.hadoop.fs.FSDataInputStream;
 import org.apache.hadoop.fs.FSDataOutputStream;
-import org.apache.hadoop.fs.FileAlreadyExistsException;
 import org.apache.hadoop.fs.FileChecksum;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
@@ -44,11 +43,9 @@
 import org.apache.hadoop.fs.FsStatus;
 import org.apache.hadoop.fs.LocatedFileStatus;
 import org.apache.hadoop.fs.Options.ChecksumOpt;
-import org.apache.hadoop.fs.ParentNotDirectoryException;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.PathFilter;
 import org.apache.hadoop.fs.RemoteIterator;
-import org.apache.hadoop.fs.UnsupportedFileSystemException;
 import org.apache.hadoop.fs.XAttrSetFlag;
 import org.apache.hadoop.fs.permission.AclEntry;
 import org.apache.hadoop.fs.permission.AclStatus;
@@ -65,7 +62,8 @@
 import org.apache.drill.shaded.guava.com.google.common.collect.Maps;
 
 /**
- * DrillFileSystem is the wrapper around the actual FileSystem implementation.
+ * DrillFileSystem is the wrapper around the actual FileSystem implementation. The {@link
DrillFileSystem} is
+ * immutable.
  *
  * If {@link org.apache.drill.exec.ops.OperatorStats} are provided it returns an instrumented
FSDataInputStream to
  * measure IO wait time and tracking file open/close operations.
@@ -88,23 +86,42 @@ public DrillFileSystem(Configuration fsConf) throws IOException {
   }
 
   public DrillFileSystem(Configuration fsConf, OperatorStats operatorStats) throws IOException
{
+    // Configuration objects are mutable, and the underlying FileSystem object may directly
use a passed in Configuration.
+    // In order to avoid scenarios where a Configuration can change after a DrillFileSystem
is created, we make a copy
+    // of the Configuration.
+    fsConf = new Configuration(fsConf);
     this.underlyingFs = FileSystem.get(fsConf);
     this.codecFactory = new CompressionCodecFactory(fsConf);
     this.operatorStats = operatorStats;
   }
 
+  private void throwUnsupported() {
+    throw new UnsupportedOperationException(DrillFileSystem.class.getCanonicalName() + "
is immutable and should not be changed after creation.");
+  }
+
+  /**
+   * This method should never be used on {@link DrillFileSystem} since {@link DrillFileSystem}
is immutable.
+   * {@inheritDoc}
+   * @throws UnsupportedOperationException when called.
+   */
   @Override
   public void setConf(Configuration conf) {
-    // Guard against setConf(null) call that is called as part of superclass constructor
(Configured) of the
-    // DrillFileSystem, at which point underlyingFs is null.
-    if (conf != null && underlyingFs != null) {
-      underlyingFs.setConf(conf);
+    if (underlyingFs != null) {
+      throwUnsupported();
     }
+
+    // The parent class's constructor FileSystem() calls Configured(null) which calls setConf(null).
+    // We want to let that first call to setConf succeed. Any subsequent calls would be made
by a user and are not allowed.
   }
 
+  /**
+   * Returns a copy of {@link Configuration} for this {@link DrillFileSystem}.
+   * <b>Note: </b> a copy of the {@link Configuration} is returned in order to
enforce immutability.
+   * @return A copy of {@link Configuration} for this {@link DrillFileSystem}.
+   */
   @Override
   public Configuration getConf() {
-    return underlyingFs.getConf();
+    return new Configuration(this.underlyingFs.getConf());
   }
 
   /**
@@ -143,9 +160,14 @@ public FSDataInputStream open(Path f) throws IOException {
     return new DrillFSDataInputStream(underlyingFs.open(f), operatorStats);
   }
 
+  /**
+   * This method should never be used on {@link DrillFileSystem} since {@link DrillFileSystem}
is immutable.
+   * {@inheritDoc}
+   * @throws UnsupportedOperationException when called.
+   */
   @Override
-  public void initialize(URI name, Configuration conf) throws IOException {
-    underlyingFs.initialize(name, conf);
+  public void initialize(URI name, Configuration conf) {
+    throwUnsupported();
   }
 
   @Override
@@ -205,13 +227,12 @@ public FileStatus getFileStatus(Path f) throws IOException {
   }
 
   @Override
-  public void createSymlink(Path target, Path link, boolean createParent) throws AccessControlException,
FileAlreadyExistsException, FileNotFoundException, ParentNotDirectoryException, UnsupportedFileSystemException,
IOException {
+  public void createSymlink(Path target, Path link, boolean createParent) throws IOException
{
     underlyingFs.createSymlink(target, link, createParent);
   }
 
   @Override
-  public FileStatus getFileLinkStatus(Path f) throws AccessControlException, FileNotFoundException,
-      UnsupportedFileSystemException, IOException {
+  public FileStatus getFileLinkStatus(Path f) throws IOException {
     return underlyingFs.getFileLinkStatus(f);
   }
 
@@ -230,14 +251,24 @@ public FileChecksum getFileChecksum(Path f) throws IOException {
     return underlyingFs.getFileChecksum(f);
   }
 
+  /**
+   * This method should never be used on {@link DrillFileSystem} since {@link DrillFileSystem}
is immutable.
+   * {@inheritDoc}
+   * @throws UnsupportedOperationException when called.
+   */
   @Override
   public void setVerifyChecksum(boolean verifyChecksum) {
-    underlyingFs.setVerifyChecksum(verifyChecksum);
+    throwUnsupported();
   }
 
+  /**
+   * This method should never be used on {@link DrillFileSystem} since {@link DrillFileSystem}
is immutable.
+   * {@inheritDoc}
+   * @throws UnsupportedOperationException when called.
+   */
   @Override
   public void setWriteChecksum(boolean writeChecksum) {
-    underlyingFs.setWriteChecksum(writeChecksum);
+    throwUnsupported();
   }
 
   @Override
@@ -567,9 +598,14 @@ public Path getHomeDirectory() {
     return underlyingFs.getHomeDirectory();
   }
 
+  /**
+   * This method should never be used on {@link DrillFileSystem} since {@link DrillFileSystem}
is immutable.
+   * {@inheritDoc}
+   * @throws UnsupportedOperationException when called.
+   */
   @Override
   public void setWorkingDirectory(Path new_dir) {
-    underlyingFs.setWorkingDirectory(new_dir);
+    throwUnsupported();
   }
 
   @Override
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
index 81e5617c4d3..f053986f309 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
@@ -212,6 +212,6 @@ public FormatPlugin getFormatPlugin(FormatPluginConfig config) {
   }
 
   public Configuration getFsConf() {
-    return fsConf;
+    return new Configuration(fsConf);
   }
 }


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message