keith-turner commented on a change in pull request #1715: URL: https://github.com/apache/accumulo/pull/1715#discussion_r501932681
########## File path: core/src/main/java/org/apache/accumulo/core/classloader/AccumuloClassLoader.java ########## @@ -0,0 +1,88 @@ +/* + * 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.accumulo.core.classloader; + +import java.lang.reflect.InvocationTargetException; + +import org.apache.accumulo.core.spi.common.ClassLoaderFactory; +import org.apache.accumulo.core.spi.common.ClassLoaderFactory.Printer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class AccumuloClassLoader { + + private static final Logger LOG = LoggerFactory.getLogger(AccumuloClassLoader.class); + + private static final String CLASS_LOADER_FACTORY = "general.class.loader.factory"; Review comment: It may be nice to have an `accumulo` prefix in this property, since its a java system property and not an Accumulo configuration property. Maybe something like `accumulo.server.class.loader.factory`. This would be similar to the way log4j prefixes its Java system properties. ########## File path: core/src/main/java/org/apache/accumulo/core/spi/common/ClassLoaderFactory.java ########## @@ -18,44 +18,33 @@ */ package org.apache.accumulo.core.spi.common; -import java.util.Collections; -import java.util.Iterator; -import java.util.Map.Entry; - /** - * The ClassLoaderFactory is defined by the property general.context.factory. The factory - * implementation is configured externally to Accumulo and will return a ClassLoader for a given - * contextName. - * + * The ClassLoaderFactory implementation is defined by the property general.class.loader.factory. Review comment: This javadoc could mention its a Java system property. ########## File path: start/src/main/java/org/apache/accumulo/start/Main.java ########## @@ -86,42 +63,42 @@ public static void main(final String[] args) { // determine whether a keyword was used or a class name, and execute it with the remaining // args String keywordOrClassName = args[0]; - KeywordExecutable keywordExec = getExecutables(loader).get(keywordOrClassName); + KeywordExecutable keywordExec = getExecutables(CLASSLOADER).get(keywordOrClassName); if (keywordExec != null) { execKeyword(keywordExec, stripArgs(args, 1)); } else { execMainClassName(keywordOrClassName, stripArgs(args, 1)); } } catch (Throwable t) { - log.error("Uncaught exception", t); + LOG.error("Uncaught exception", t); System.exit(1); } } - public static synchronized ClassLoader getClassLoader() { - if (classLoader == null) { - try { - classLoader = (ClassLoader) getVFSClassLoader().getMethod("getClassLoader").invoke(null); - Thread.currentThread().setContextClassLoader(classLoader); - } catch (IOException | IllegalArgumentException | ReflectiveOperationException - | SecurityException e) { - log.error("Problem initializing the class loader", e); - System.exit(1); - } - } - return classLoader; - } - - public static synchronized Class<?> getVFSClassLoader() - throws IOException, ClassNotFoundException { - if (vfsClassLoader == null) { - Thread.currentThread().setContextClassLoader(AccumuloClassLoader.getClassLoader()); - vfsClassLoader = AccumuloClassLoader.getClassLoader() - .loadClass("org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader"); - } - return vfsClassLoader; - } + // public static synchronized ClassLoader getClassLoader() { Review comment: Why is this commented out code here? ########## File path: core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java ########## @@ -0,0 +1,60 @@ +/* + * 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.accumulo.core.spi.common; + +import java.util.Collections; +import java.util.Iterator; +import java.util.Map.Entry; + +/** + * The ContextClassLoaderFactory implementation is defined by the property + * general.context.class.loader.factory. The implementation is configured externally to Accumulo and + * will return a ClassLoader for a given contextName. + */ +public interface ContextClassLoaderFactory { + + static class ClassLoaderFactoryConfiguration { + + public Iterator<Entry<String,String>> get() { + return Collections.emptyIterator(); + } + } + + /** + * Initialize the ClassLoaderFactory. Implementations may need a reference to the configuration so + * that it can clean up contexts that are no longer being used. + * + * @param conf + * Accumulo configuration properties + * @throws Exception + * if error initializing ClassLoaderFactory + */ + void initialize(ClassLoaderFactoryConfiguration conf) throws Exception; Review comment: Could name the class InitializationParameters to be consistent w/ other SPI interfaces. ########## File path: core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java ########## @@ -0,0 +1,60 @@ +/* + * 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.accumulo.core.spi.common; + +import java.util.Collections; +import java.util.Iterator; +import java.util.Map.Entry; + +/** + * The ContextClassLoaderFactory implementation is defined by the property + * general.context.class.loader.factory. The implementation is configured externally to Accumulo and + * will return a ClassLoader for a given contextName. + */ +public interface ContextClassLoaderFactory { + + static class ClassLoaderFactoryConfiguration { + + public Iterator<Entry<String,String>> get() { Review comment: Other SPI interfaces use ServiceEnvironment to expose Accumulo configuration to plugins. https://github.com/apache/accumulo/blob/80ee9ca8092dc4bf07aa1a046d6e668f0e16300e/core/src/main/java/org/apache/accumulo/core/spi/common/ServiceEnvironment.java#L35 ########## File path: core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java ########## @@ -0,0 +1,60 @@ +/* + * 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.accumulo.core.spi.common; + +import java.util.Collections; +import java.util.Iterator; +import java.util.Map.Entry; + +/** + * The ContextClassLoaderFactory implementation is defined by the property + * general.context.class.loader.factory. The implementation is configured externally to Accumulo and + * will return a ClassLoader for a given contextName. Review comment: Javadoc needs a since tag ########## File path: core/src/main/java/org/apache/accumulo/core/spi/common/ClassLoaderFactory.java ########## @@ -18,44 +18,33 @@ */ package org.apache.accumulo.core.spi.common; -import java.util.Collections; -import java.util.Iterator; -import java.util.Map.Entry; - /** - * The ClassLoaderFactory is defined by the property general.context.factory. The factory - * implementation is configured externally to Accumulo and will return a ClassLoader for a given - * contextName. - * + * The ClassLoaderFactory implementation is defined by the property general.class.loader.factory. + * The implementation will return a ClassLoader to be used for dynamically loading classes. */ public interface ClassLoaderFactory { - static class ClassLoaderFactoryConfiguration { - - public Iterator<Entry<String,String>> get() { - return Collections.emptyIterator(); - } + public interface Printer { + void print(String s); } /** - * Initialize the ClassLoaderFactory. Implementations may need a reference to the configuration so - * that it can clean up contexts that are no longer being used. + * Return the configured classloader * - * @param conf - * Accumulo configuration properties - * @throws Exception - * if error initializing ClassLoaderFactory + * @return classloader the configured classloader */ - void initialize(ClassLoaderFactoryConfiguration conf) throws Exception; + ClassLoader getClassLoader() throws Exception; /** + * Print the classpath to the Printer * - * @param contextName - * name of classloader context - * @return classloader configured for the context - * @throws IllegalArgumentException - * if contextName is not supported + * @param cl + * classloader + * @param out + * printer + * @param debug + * enable debug output */ - ClassLoader getClassLoader(String contextName) throws IllegalArgumentException; + void printClassPath(ClassLoader cl, Printer out, boolean debug); Review comment: For SPI methods that take multiple arguments other SPI interfaces use Parameters interfaces. This makes it easy to add more parameters to the method in the future w/o causing issues for existing implementers. Below is one example of this. https://github.com/apache/accumulo/blob/80ee9ca8092dc4bf07aa1a046d6e668f0e16300e/core/src/main/java/org/apache/accumulo/core/spi/scan/ScanDispatcher.java#L62 ########## File path: core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java ########## @@ -0,0 +1,60 @@ +/* + * 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.accumulo.core.spi.common; + +import java.util.Collections; +import java.util.Iterator; +import java.util.Map.Entry; + +/** + * The ContextClassLoaderFactory implementation is defined by the property + * general.context.class.loader.factory. The implementation is configured externally to Accumulo and + * will return a ClassLoader for a given contextName. + */ +public interface ContextClassLoaderFactory { + + static class ClassLoaderFactoryConfiguration { Review comment: I suspect this needs javadoc w/ since tag. I don't think Inner classes inherit since tags from outer classes javadoc, but not 100% sure. ########## File path: core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java ########## @@ -0,0 +1,32 @@ +/* + * 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.accumulo.core.classloader; + +public class ClassLoaderUtil { + + public static synchronized <U> Class<? extends U> loadClass(String contextName, String className, Review comment: In the past when running test against Accumulo w/ lots of concurrent scans and profiling tablets servers, the static synchronization around class loading was something that showed up in profiling data. Every scan uses class loaders to create iterators and this static synch causes locking contention between scans. This in an existing problem with the current code and I don't think changes are needed here, seeing this reminded me of the problem. I need to see if I opened an issue for this. ########## File path: core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java ########## @@ -0,0 +1,60 @@ +/* + * 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.accumulo.core.spi.common; + +import java.util.Collections; +import java.util.Iterator; +import java.util.Map.Entry; + +/** + * The ContextClassLoaderFactory implementation is defined by the property + * general.context.class.loader.factory. The implementation is configured externally to Accumulo and + * will return a ClassLoader for a given contextName. + */ +public interface ContextClassLoaderFactory { + + static class ClassLoaderFactoryConfiguration { Review comment: I think all members of interfaces are implicitly public. ########## File path: core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java ########## @@ -0,0 +1,32 @@ +/* + * 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.accumulo.core.classloader; + +public class ClassLoaderUtil { + + public static synchronized <U> Class<? extends U> loadClass(String contextName, String className, Review comment: Looking into this overall issue a bit further in the existing code, iterators call AccumuloVFSClassLoader.loadClass() which is static and synchronized. Also the iterator loading code has a class cache, maybe this was created to avoid the global synchronization. Also I did not find an issue, will open one after looking into it a bit more. ########## File path: core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java ########## @@ -0,0 +1,32 @@ +/* + * 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.accumulo.core.classloader; + +public class ClassLoaderUtil { + + public static synchronized <U> Class<? extends U> loadClass(String contextName, String className, Review comment: The cache is only narrowly used, so the synchronization is probably still problematic I'll open an issue. ########## File path: core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java ########## @@ -0,0 +1,32 @@ +/* + * 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.accumulo.core.classloader; + +public class ClassLoaderUtil { + + public static synchronized <U> Class<? extends U> loadClass(String contextName, String className, Review comment: Created #1730 ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
