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:
[email protected]
With regards,
Apache Git Services
