[GitHub] sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin config from changing default fs. Make DrillFileSystem Immutable.
sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin
config from changing default fs. Make DrillFileSystem Immutable.
URL: https://github.com/apache/drill/pull/1296#discussion_r213507759
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
##
@@ -79,32 +77,56 @@
private final ConcurrentMap
openedFiles = Maps.newConcurrentMap();
+ private final Configuration fsConf;
private final FileSystem underlyingFs;
private final OperatorStats operatorStats;
private final CompressionCodecFactory codecFactory;
+ private boolean initialized = false;
+
public DrillFileSystem(Configuration fsConf) throws IOException {
this(fsConf, null);
}
public DrillFileSystem(Configuration fsConf, OperatorStats operatorStats)
throws IOException {
-this.underlyingFs = FileSystem.get(fsConf);
-this.codecFactory = new CompressionCodecFactory(fsConf);
+// 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.
+this.fsConf = new Configuration(fsConf);
+this.underlyingFs = FileSystem.get(this.fsConf);
+this.codecFactory = new CompressionCodecFactory(this.fsConf);
this.operatorStats = operatorStats;
+this.initialized = true;
+ }
+
+ 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 (!initialized) {
Review comment:
We can still use `(underlyingFs != null)` check and get rid of the
`initialized` flag. New check can be:
```
if (underlyingFs != null) {
throwUnsupported();
}
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin config from changing default fs. Make DrillFileSystem Immutable.
sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin
config from changing default fs. Make DrillFileSystem Immutable.
URL: https://github.com/apache/drill/pull/1296#discussion_r213508890
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
##
@@ -79,32 +77,56 @@
private final ConcurrentMap
openedFiles = Maps.newConcurrentMap();
+ private final Configuration fsConf;
private final FileSystem underlyingFs;
private final OperatorStats operatorStats;
private final CompressionCodecFactory codecFactory;
+ private boolean initialized = false;
+
public DrillFileSystem(Configuration fsConf) throws IOException {
this(fsConf, null);
}
public DrillFileSystem(Configuration fsConf, OperatorStats operatorStats)
throws IOException {
-this.underlyingFs = FileSystem.get(fsConf);
-this.codecFactory = new CompressionCodecFactory(fsConf);
+// 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.
+this.fsConf = new Configuration(fsConf);
Review comment:
We can convert `this.fsConf` to be a local variable and in `getConf()` do
`return new Configuration(underlyingFs.getConf());`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin config from changing default fs. Make DrillFileSystem Immutable.
sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin
config from changing default fs. Make DrillFileSystem Immutable.
URL: https://github.com/apache/drill/pull/1296#discussion_r204180625
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
##
@@ -65,46 +62,75 @@
import 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.
*/
public class DrillFileSystem extends FileSystem implements OpenFileTracker {
static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillFileSystem.class);
private final static boolean TRACKING_ENABLED =
AssertionUtil.isAssertionsEnabled();
+ private final static DrillFileSystemCache CACHE = new DrillFileSystemCache();
+ public static final String FS_DEFAULT_NAME = "fs.default.name";
public static final String UNDERSCORE_PREFIX = "_";
public static final String DOT_PREFIX = ".";
private final ConcurrentMap
openedFiles = Maps.newConcurrentMap();
+ private final Configuration fsConf;
private final FileSystem underlyingFs;
private final OperatorStats operatorStats;
private final CompressionCodecFactory codecFactory;
+ private boolean initialized = false;
+
public DrillFileSystem(Configuration fsConf) throws IOException {
this(fsConf, null);
}
public DrillFileSystem(Configuration fsConf, OperatorStats operatorStats)
throws IOException {
-this.underlyingFs = FileSystem.get(fsConf);
+// 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.
+this.fsConf = new Configuration(fsConf);
+fsConf.setDeprecatedProperties();
Review comment:
I don't think this is needed because deprecation properties is handled for
each `set` call on configuration object. See
[here](https://github.com/apache/hadoop/blob/branch-2.7.1/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java#L1138)
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin config from changing default fs. Make DrillFileSystem Immutable.
sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin
config from changing default fs. Make DrillFileSystem Immutable.
URL: https://github.com/apache/drill/pull/1296#discussion_r204185012
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystemCache.java
##
@@ -0,0 +1,99 @@
+/*
+ * 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
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.dfs;
+
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Motivation
+ *
+ * This cache is intended to work around the bugs in the {@link
org.apache.hadoop.fs.FileSystem} static cache (DRILL-5365). Specifically, as of
Hadoop 2.7.x the
+ * {@link org.apache.hadoop.fs.FileSystem} cache has the following bad
behavior:
+ *
+ *
+ *
+ * The {@link org.apache.hadoop.conf.Configuration} object is not
considered when constructing keys for the {@link
org.apache.hadoop.fs.FileSystem} cache of
+ * {@link org.apache.hadoop.fs.FileSystem} objects.
+ *
+ *
+ * The {@link org.apache.hadoop.fs.FileSystem} cache does not honor the
fs.default.name property when constructing keys, only
fs.defaultFS is used to construct
+ * keys in the cache.
+ *
+ *
+ *
+ * Usage
+ *
+ *
+ *
+ * A prerequisite for usage is that all {@link
org.apache.hadoop.conf.Configuration} objects are normalized with
+ * {@link
org.apache.drill.exec.store.dfs.DrillFileSystem#normalize(Configuration)}.
+ *
+ *
+ * This cache should only be used from {@link
org.apache.drill.exec.store.dfs.DrillFileSystem}.
+ *
+ *
+ *
+ * TODO
+ *
+ *
+ *
+ * Drill currently keeps a {@link
org.apache.drill.exec.store.dfs.DrillFileSystem} open indefinitely. This will
be corrected
+ * in DRILL-6608. As a result this cache currently has no methods to
remove {@link org.apache.hadoop.fs.FileSystem} objects
+ * after they are created.
+ *
+ *
+ */
+class DrillFileSystemCache {
+ private Map, FileSystem> cache = new HashMap<>();
+
+ /**
+ * If a {@link org.apache.hadoop.fs.FileSystem} object corresponding to the
given {@link org.apache.hadoop.conf.Configuration}
+ * exists in the cache, then it is returned. If no corresponding {@link
org.apache.hadoop.fs.FileSystem} exist, then it is created,
+ * added to the cache, and returned.
+ * @param configuration The {@link org.apache.hadoop.conf.Configuration}
corresponding to the desired {@link org.apache.hadoop.fs.FileSystem}
+ * object. It is expected that this configuration is
first normalized with {@link
org.apache.drill.exec.store.dfs.DrillFileSystem#normalize(Configuration)}.
+ * @return The {@link org.apache.hadoop.fs.FileSystem} object corresponding
to he given {@link org.apache.hadoop.conf.Configuration}.
+ * @throws IOException An error when creating the desired {@link
org.apache.hadoop.fs.FileSystem} object.
+ */
+ protected synchronized FileSystem get(final Configuration configuration)
throws IOException {
+final Map map = new HashMap<>(configToMap(configuration));
+
+if (!cache.containsKey(map)) {
+ cache.put(map, FileSystem.newInstance(configuration));
+}
+
+return cache.get(map);
+ }
+
+ static Map configToMap(final Configuration configuration) {
+Preconditions.checkNotNull(configuration);
+final Map map = new HashMap<>();
+
+for (Map.Entry entry: configuration) {
+ map.put(entry.getKey().trim(), entry.getValue());
Review comment:
Makes sense and probably that's the reason `Cache` only uses `Scheme` and
`authority` properties which are derived from `fs.defaultFS` parameter. I guess
the proxy servers are provided as `authority` strings. All Kerberos and
security related parameters are encapsulated within `UGI` object.
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
[GitHub] sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin config from changing default fs. Make DrillFileSystem Immutable.
sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin
config from changing default fs. Make DrillFileSystem Immutable.
URL: https://github.com/apache/drill/pull/1296#discussion_r204170180
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystemCache.java
##
@@ -0,0 +1,99 @@
+/*
+ * 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
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store.dfs;
+
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Motivation
+ *
+ * This cache is intended to work around the bugs in the {@link
org.apache.hadoop.fs.FileSystem} static cache (DRILL-5365). Specifically, as of
Hadoop 2.7.x the
+ * {@link org.apache.hadoop.fs.FileSystem} cache has the following bad
behavior:
+ *
+ *
+ *
+ * The {@link org.apache.hadoop.conf.Configuration} object is not
considered when constructing keys for the {@link
org.apache.hadoop.fs.FileSystem} cache of
+ * {@link org.apache.hadoop.fs.FileSystem} objects.
+ *
+ *
+ * The {@link org.apache.hadoop.fs.FileSystem} cache does not honor the
fs.default.name property when constructing keys, only
fs.defaultFS is used to construct
+ * keys in the cache.
+ *
+ *
+ *
+ * Usage
+ *
+ *
+ *
+ * A prerequisite for usage is that all {@link
org.apache.hadoop.conf.Configuration} objects are normalized with
+ * {@link
org.apache.drill.exec.store.dfs.DrillFileSystem#normalize(Configuration)}.
+ *
+ *
+ * This cache should only be used from {@link
org.apache.drill.exec.store.dfs.DrillFileSystem}.
+ *
+ *
+ *
+ * TODO
+ *
+ *
+ *
+ * Drill currently keeps a {@link
org.apache.drill.exec.store.dfs.DrillFileSystem} open indefinitely. This will
be corrected
+ * in DRILL-6608. As a result this cache currently has no methods to
remove {@link org.apache.hadoop.fs.FileSystem} objects
+ * after they are created.
+ *
+ *
+ */
+class DrillFileSystemCache {
+ private Map, FileSystem> cache = new HashMap<>();
+
+ /**
+ * If a {@link org.apache.hadoop.fs.FileSystem} object corresponding to the
given {@link org.apache.hadoop.conf.Configuration}
+ * exists in the cache, then it is returned. If no corresponding {@link
org.apache.hadoop.fs.FileSystem} exist, then it is created,
+ * added to the cache, and returned.
+ * @param configuration The {@link org.apache.hadoop.conf.Configuration}
corresponding to the desired {@link org.apache.hadoop.fs.FileSystem}
+ * object. It is expected that this configuration is
first normalized with {@link
org.apache.drill.exec.store.dfs.DrillFileSystem#normalize(Configuration)}.
+ * @return The {@link org.apache.hadoop.fs.FileSystem} object corresponding
to he given {@link org.apache.hadoop.conf.Configuration}.
+ * @throws IOException An error when creating the desired {@link
org.apache.hadoop.fs.FileSystem} object.
+ */
+ protected synchronized FileSystem get(final Configuration configuration)
throws IOException {
+final Map map = new HashMap<>(configToMap(configuration));
+
+if (!cache.containsKey(map)) {
+ cache.put(map, FileSystem.newInstance(configuration));
+}
+
+return cache.get(map);
+ }
+
+ static Map configToMap(final Configuration configuration) {
+Preconditions.checkNotNull(configuration);
+final Map map = new HashMap<>();
+
+for (Map.Entry entry: configuration) {
+ map.put(entry.getKey().trim(), entry.getValue());
Review comment:
- That constructor calls `this(uri, conf, 0)`
(https://github.com/apache/hadoop/blob/branch-2.7.1/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java#L2802)
which calls `UGI.getCurrentUser()`. The user returned by this call is stored
in Key. This call will return correct user only if the caller of FileSystem
constructor is called under correct user context which is done in UGI.doAs
block otherwise it returns process user. I think I was not able to clarify the
usage of `UGI.doAs block`. Please see [this
[GitHub] sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin config from changing default fs. Make DrillFileSystem Immutable.
sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin
config from changing default fs. Make DrillFileSystem Immutable.
URL: https://github.com/apache/drill/pull/1296#discussion_r203944690
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
##
@@ -65,46 +62,105 @@
import 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.
*/
public class DrillFileSystem extends FileSystem implements OpenFileTracker {
static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillFileSystem.class);
private final static boolean TRACKING_ENABLED =
AssertionUtil.isAssertionsEnabled();
+ private final static DrillFileSystemCache CACHE = new DrillFileSystemCache();
+ public static final String FS_DEFAULT_NAME = "fs.default.name";
public static final String UNDERSCORE_PREFIX = "_";
public static final String DOT_PREFIX = ".";
private final ConcurrentMap
openedFiles = Maps.newConcurrentMap();
+ private final Configuration fsConf;
private final FileSystem underlyingFs;
private final OperatorStats operatorStats;
private final CompressionCodecFactory codecFactory;
+ private boolean initialized = false;
+
public DrillFileSystem(Configuration fsConf) throws IOException {
this(fsConf, null);
}
public DrillFileSystem(Configuration fsConf, OperatorStats operatorStats)
throws IOException {
-this.underlyingFs = FileSystem.get(fsConf);
+// 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.
+this.fsConf = new Configuration(fsConf);
+normalize(fsConf);
Review comment:
Please see comment below `fs.defaultFS` is new property and
f`s.default.name` is the deprecated one. I can look more on how to actually use
deprecatedContext but my assumption was internally FileSystem library must be
initializing the mapping between deprecated and new properties and use new
properties value correctly. Also `fs.defaultFS` will always be set since it has
a default value of `file:///`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin config from changing default fs. Make DrillFileSystem Immutable.
sohami commented on a change in pull request #1296: DRILL-5365: Prevent plugin
config from changing default fs. Make DrillFileSystem Immutable.
URL: https://github.com/apache/drill/pull/1296#discussion_r203504920
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/DrillFileSystem.java
##
@@ -65,46 +62,105 @@
import 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.
*/
public class DrillFileSystem extends FileSystem implements OpenFileTracker {
static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillFileSystem.class);
private final static boolean TRACKING_ENABLED =
AssertionUtil.isAssertionsEnabled();
+ private final static DrillFileSystemCache CACHE = new DrillFileSystemCache();
+ public static final String FS_DEFAULT_NAME = "fs.default.name";
public static final String UNDERSCORE_PREFIX = "_";
public static final String DOT_PREFIX = ".";
private final ConcurrentMap
openedFiles = Maps.newConcurrentMap();
+ private final Configuration fsConf;
private final FileSystem underlyingFs;
private final OperatorStats operatorStats;
private final CompressionCodecFactory codecFactory;
+ private boolean initialized = false;
+
public DrillFileSystem(Configuration fsConf) throws IOException {
this(fsConf, null);
}
public DrillFileSystem(Configuration fsConf, OperatorStats operatorStats)
throws IOException {
-this.underlyingFs = FileSystem.get(fsConf);
+// 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.
+this.fsConf = new Configuration(fsConf);
+normalize(fsConf);
Review comment:
Why not use `Configuration.setDeprecateProperties` instead ? See
[here](https://github.com/apache/hadoop/blob/branch-2.7.1/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java#L577)
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:
us...@infra.apache.org
With regards,
Apache Git Services
