[
https://issues.apache.org/jira/browse/HADOOP-18993?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17792403#comment-17792403
]
ASF GitHub Bot commented on HADOOP-18993:
-----------------------------------------
steveloughran commented on code in PR #6301:
URL: https://github.com/apache/hadoop/pull/6301#discussion_r1412838038
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.hadoop.fs.s3a;
+
+import java.io.IOException;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+
+import org.apache.hadoop.conf.Configuration;
+import static org.assertj.core.api.Assertions.*;
+import org.junit.Test;
+
+public class ITestS3AFileSystemIsolatedClassloader extends AbstractS3ATestBase
{
+
+ public static class CustomClassLoader extends ClassLoader {
+ }
+
+ private final ClassLoader customClassLoader = new CustomClassLoader();
+
+ private <T> T createClass(ClassLoader clsLoader, Class<T> toLoad) {
+ try {
+ Class<?> cls = clsLoader.loadClass(toLoad.getCanonicalName());
+ Constructor<?> constructor = cls.getConstructor();
+ return (T) constructor.newInstance();
+ } catch(ClassNotFoundException | InstantiationException |
IllegalAccessException |
+ IllegalArgumentException | InvocationTargetException |
NoSuchMethodException |
+ SecurityException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Test
+ public void isolatedClassloader() throws IOException {
+ try (S3AFileSystem fs = getFileSystem()) {
Review Comment:
this isn't going to work right. the fs is already initialized at that point;
double initing is trouble (maybe we should reject it?). better to create a new
S3AFileSystem() then initialize it
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java:
##########
@@ -1360,4 +1360,22 @@ private Constants() {
* Value: {@value}.
*/
public static final boolean OPTIMIZED_COPY_FROM_LOCAL_DEFAULT = true;
+
+
+ /**
+ * To use the same classloader that loaded S3AFileSystem to load
+ * user extensions, such as {{fs.s3a.aws.credentials.provider}}.
+ * It is useful to turn this off for Apache Spark applications that
+ * might load S3AFileSystem from the Spark distribution (Launcher
classloader)
+ * while users might want to provide custom extensions (loaded by Spark
MutableClassloader).
+ * Default value: true.
Review Comment:
add a line stating the value, as IDEs show these
```
* Value: {@value}
```
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java:
##########
@@ -1360,4 +1360,22 @@ private Constants() {
* Value: {@value}.
*/
public static final boolean OPTIMIZED_COPY_FROM_LOCAL_DEFAULT = true;
+
+
+ /**
+ * To use the same classloader that loaded S3AFileSystem to load
+ * user extensions, such as {{fs.s3a.aws.credentials.provider}}.
Review Comment:
use {@code fs.s3a.aws.credentials.provider} for the formatting
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.hadoop.fs.s3a;
+
+import java.io.IOException;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+
+import org.apache.hadoop.conf.Configuration;
+import static org.assertj.core.api.Assertions.*;
Review Comment:
check the hadoop ordering rules here
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AFileSystemIsolatedClassloader.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.hadoop.fs.s3a;
+
+import java.io.IOException;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+
+import org.apache.hadoop.conf.Configuration;
+import static org.assertj.core.api.Assertions.*;
+import org.junit.Test;
+
+public class ITestS3AFileSystemIsolatedClassloader extends AbstractS3ATestBase
{
+
+ public static class CustomClassLoader extends ClassLoader {
+ }
+
+ private final ClassLoader customClassLoader = new CustomClassLoader();
+
+ private <T> T createClass(ClassLoader clsLoader, Class<T> toLoad) {
+ try {
+ Class<?> cls = clsLoader.loadClass(toLoad.getCanonicalName());
+ Constructor<?> constructor = cls.getConstructor();
+ return (T) constructor.newInstance();
+ } catch(ClassNotFoundException | InstantiationException |
IllegalAccessException |
+ IllegalArgumentException | InvocationTargetException |
NoSuchMethodException |
+ SecurityException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Test
+ public void isolatedClassloader() throws IOException {
+ try (S3AFileSystem fs = getFileSystem()) {
+ Configuration conf = createClass(customClassLoader, Configuration.class);
+ assertEquals(fs.getConf().getClassLoader(), customClassLoader);
Review Comment:
use AssertJ asserts. They are more detailed and avoid the bug your asserts
here have, in that the expected vs actual parameters are in the wrong order -so
the error text will be misleading.
rather than fix up these asserts, just embrace AssertJ, which we are
adopting across all new hadoop-aws test case. thanks
> Allow to not isolate S3AFileSystem classloader when needed
> ----------------------------------------------------------
>
> Key: HADOOP-18993
> URL: https://issues.apache.org/jira/browse/HADOOP-18993
> Project: Hadoop Common
> Issue Type: Improvement
> Components: hadoop-thirdparty
> Affects Versions: 3.3.6
> Reporter: Antonio Murgia
> Priority: Minor
> Labels: pull-request-available
> Fix For: 3.4.0
>
>
> In HADOOP-17372 the S3AFileSystem forces the configuration classloader to be
> the same as the one that loaded S3AFileSystem. This leads to the
> impossibility in Spark applications to load third party credentials providers
> as user jars.
> I propose to add a configuration key
> {{fs.s3a.extensions.isolated.classloader}} with a default value of {{true}}
> that if set to {{false}} will not perform the classloader set.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
