> On July 7, 2016, 4:40 p.m., Shwetha GS wrote:
> > 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());
> > }
> > });
> > }
Thanks for the unit test! Added in this revision.
> On July 7, 2016, 4:40 p.m., Shwetha GS wrote:
> > addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java,
> > line 47
> > <https://reviews.apache.org/r/49585/diff/3/?file=1436632#file1436632line47>
> >
> > getName() can return this.getClass.getName. Doesn't need to re-direct
> > to plugin
It will be cleaner for shim to not short-circuit the implementation of the
class being shimmed. I would suggest leaving this as it is.
> On July 7, 2016, 4:40 p.m., Shwetha GS wrote:
> > plugin-classloader/src/main/java/org/apache/atlas/plugin/classloader/AtlasPluginClassLoader.java,
> > line 64
> > <https://reviews.apache.org/r/49585/diff/3/?file=1436644#file1436644line64>
> >
> > 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
Having debug level logs will help troubleshoot in production environments when
needed. Since the default log level would be INFO, these logs will not be seen
in production environments. In my experience, having more debug level
statements have been extremely helpful to troubleshoot, which help avoid having
to provide instrumented JARs. I would suggest leaving the debug statements in
place.
> On July 7, 2016, 4:40 p.m., Shwetha GS wrote:
> > addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java,
> > line 181
> > <https://reviews.apache.org/r/49585/diff/3/?file=1436632#file1436632line181>
> >
> > This is a common code across all hooks. Move to utility in
> > plugin-classloader?
This line instantiates AtlasPluginClassLoader. Instead of using a constructor,
a call to AtlasPluginClassLoader.getInstance() is being made. What does it mean
to move this to plugin-classloader? Also, ATLAS_PLUGIN_TYPE is different for
each hook.
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49585/#review141174
-----------------------------------------------------------
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
>
>
