-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49585/#review141174
-----------------------------------------------------------
Here is the test that works:
public void testClassLoader() throws Exception {
String cls = "org.apache.atlas.service.Services";
try {
loadClass(cls, null);
fail("Expected ClassNotFoundException");
} catch(ClassNotFoundException e) {
//expected
}
AtlasPluginClassLoader classLoader = new
AtlasPluginClassLoader("../common/target");
classLoader.activate();
//org.apache.atlas.service.Services class should be loadable now
//should also load org.apache.atlas.service.Service
Class<?> servicesCls = loadClass(cls, null);
loadClass("org.apache.atlas.service.Service",
servicesCls.getClassLoader());
//Fall back to current class loader should also work
loadClass(AtlasPluginClassLoaderUtil.class.getName(), null);
classLoader.deactivate();
//After disable, class loading should fail again
try {
loadClass(cls, null);
fail("Expected ClassNotFoundException");
} catch(ClassNotFoundException e) {
//expected
}
}
private Class<?> loadClass(String cls, ClassLoader classLoader) throws
ClassNotFoundException {
if (classLoader == null) {
classLoader = Thread.currentThread().getContextClassLoader();
}
return Class.forName(cls, true, classLoader);
}
Also, modified the constructors:
private AtlasPluginClassLoader(String pluginType, Class<?> pluginClass)
throws Exception {
this(AtlasPluginClassLoaderUtil.getPluginImplLibPath(pluginType,
pluginClass));
}
//visible for testing
AtlasPluginClassLoader(String libraryPath) throws Exception {
super(AtlasPluginClassLoaderUtil.getPluginFilesForPluginTypeAndPluginclass(libraryPath),
null);
componentClassLoader = AccessController.doPrivileged(new
PrivilegedAction<MyClassLoader>() {
public MyClassLoader run() {
return new
MyClassLoader(Thread.currentThread().getContextClassLoader());
}
});
}
addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java
(line 47)
<https://reviews.apache.org/r/49585/#comment206588>
getName() can return this.getClass.getName. Doesn't need to re-direct to
plugin
addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java
(line 181)
<https://reviews.apache.org/r/49585/#comment206589>
This is a common code across all hooks. Move to utility in
plugin-classloader?
plugin-classloader/pom.xml (line 38)
<https://reviews.apache.org/r/49585/#comment206590>
already in main pom, so not required
plugin-classloader/src/main/java/org/apache/atlas/plugin/classloader/AtlasPluginClassLoader.java
(line 64)
<https://reviews.apache.org/r/49585/#comment206591>
findClass and loadClass are called a lot of times in the component and this
prints way too many logs. For debugging, -verbose:class gives enough
information. So, can you remove these debug statements?
Also, consider removing debug statements in other methods as well. Don't
think we need at both entry and exit of every method
plugin-classloader/src/main/java/org/apache/atlas/plugin/classloader/AtlasPluginClassLoader.java
(line 97)
<https://reviews.apache.org/r/49585/#comment206592>
is synchronised required here?
plugin-classloader/src/main/java/org/apache/atlas/plugin/classloader/AtlasPluginClassLoaderUtil.java
(line 44)
<https://reviews.apache.org/r/49585/#comment206594>
getInstance() not required for utility class. Make the methods static
instead
- Shwetha GS
On July 5, 2016, 1:24 a.m., Madhan Neethiraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49585/
> -----------------------------------------------------------
>
> (Updated July 5, 2016, 1:24 a.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-987
> https://issues.apache.org/jira/browse/ATLAS-987
>
>
> Repository: atlas
>
>
> Description
> -------
>
> Dependent libraries of Atlas hooks are loaded in a separate class loader,
> there by making these libraries not visibile to components.
>
> Here is the summary of the changes:
> - current contents of atlas/hook/<component> directiries, including the
> hook class implementation, are moved under
> atlas/hook/<component>/atlas-<component>-plugin-impl directory
> - a new component specific shim library, that includes the hook class
> registered with the component, is placed in directory atlas/hook/<component>
> - the hook class in the shim library loads all files in
> atlas-<component>-plugin-impl directory in a classloader and forwards all the
> calls on the shim class to the real implementation class
>
> This implementation is same as the one used in Apache Ranger - more details
> in RANGER-586.
>
>
> Diffs
> -----
>
> addons/falcon-bridge-shim/pom.xml PRE-CREATION
>
> addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java
> PRE-CREATION
> addons/falcon-bridge/pom.xml d79dda9
> addons/hive-bridge-shim/pom.xml PRE-CREATION
>
> addons/hive-bridge-shim/src/main/java/org/apache/atlas/hive/hook/HiveHook.java
> PRE-CREATION
> addons/hive-bridge/pom.xml ddefdc2
> addons/sqoop-bridge-shim/pom.xml PRE-CREATION
>
> addons/sqoop-bridge-shim/src/main/java/org/apache/atlas/sqoop/hook/SqoopHook.java
> PRE-CREATION
> addons/sqoop-bridge/pom.xml c792945
> addons/storm-bridge-shim/pom.xml PRE-CREATION
>
> addons/storm-bridge-shim/src/main/java/org/apache/atlas/storm/hook/StormAtlasHook.java
> PRE-CREATION
> addons/storm-bridge/pom.xml 9e8bf2f
> plugin-classloader/pom.xml PRE-CREATION
>
> plugin-classloader/src/main/java/org/apache/atlas/plugin/classloader/AtlasPluginClassLoader.java
> PRE-CREATION
>
> plugin-classloader/src/main/java/org/apache/atlas/plugin/classloader/AtlasPluginClassLoaderUtil.java
> PRE-CREATION
> pom.xml f119525
>
> Diff: https://reviews.apache.org/r/49585/diff/
>
>
> Testing
> -------
>
> Verified that existing tests pass
> Verified Hive hook successfully sends notification to Kafka, which in turn
> was processed correctly by Atlas server
>
>
> Thanks,
>
> Madhan Neethiraj
>
>
