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 Tue, 05 Jul 2016 16:55:11 GMT


> On July 4, 2016, 10:13 a.m., Shwetha GS wrote:
> > addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java,
line 46
> > <https://reviews.apache.org/r/49585/diff/2/?file=1436099#file1436099line46>
> >
> >     We should use aspectj and remove loggers and activate and deactive. If you can't
address in this patch, can you file a bug?

The comment does not seem related to this source line. Can you review?


> On July 4, 2016, 10:13 a.m., Shwetha GS wrote:
> > addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java,
line 31
> > <https://reviews.apache.org/r/49585/diff/2/?file=1436099#file1436099line31>
> >
> >     Does falcon-shim/AtlasService and falcon/AtasService need to have same name
and package? If not, can you rename them as its confusing
> >     
> >     For example, falcon-shim/AtlasService can be renamed to AtlasProxyService

With the use of classname/package for the shim and the implementation, no changes are needed
in the hook registration in components (Falcon/Hive/Sqoop/Storm). Hence this choice.


> On July 4, 2016, 10:13 a.m., Shwetha GS wrote:
> > addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java,
line 183
> > <https://reviews.apache.org/r/49585/diff/2/?file=1436099#file1436099line183>
> >
> >     Shouldn't Class.forName also be after activate?

The implementation class should be present in the pluginClassLoader. Hence it is okay (or
better) to call Class.forName() before activate().


> On July 4, 2016, 10:13 a.m., Shwetha GS wrote:
> > addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java,
line 190
> > <https://reviews.apache.org/r/49585/diff/2/?file=1436099#file1436099line190>
> >
> >     Extra field variable is un-necessary

AtlasService implements 2 interfaces - FalconService and ConfigurationChangeListener. Hence
using 2 different member variables, one for each interface - to avoid having to cast for each
call.


> On July 4, 2016, 10:13 a.m., Shwetha GS wrote:
> > addons/falcon-bridge/pom.xml, line 199
> > <https://reviews.apache.org/r/49585/diff/2/?file=1436100#file1436100line199>
> >
> >     The reason we added these explicit list of jars is because we had class conflict
issues. But this explicit list is hard to maintain as it changes across different versions
of falcon for example. With this patch, anyway we have different class loader, we should just
copy all the transitive dependencies that we need using copy-dependencies goal. Of course,
we can make the falcon dependencies as provided so that we don't copy even falcon jars.

Can we do this is a subsequent patch?


- Madhan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49585/#review140632
-----------------------------------------------------------


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