atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Madhan Neethiraj <mad...@apache.org>
Subject Re: Review Request 49585: ATLAS-987: load dependent libraries of Atlas hooks in a separate class loader to avoid adding them to component CLASSPATH
Date Thu, 07 Jul 2016 17:30:29 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message