atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shwetha GS <sshivalingamur...@hortonworks.com>
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 Mon, 04 Jul 2016 10:13:48 GMT

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



Have review falcon-bridge. But the same comments apply to other bridges as well.

Enable checkstyle for the new modules by adding property - <checkstyle.failOnViolation>true</checkstyle.failOnViolation>


addons/falcon-bridge-shim/pom.xml (line 42)
<https://reviews.apache.org/r/49585/#comment206011>

    remove version and move to dependency management in parent pom



addons/falcon-bridge-shim/pom.xml (line 46)
<https://reviews.apache.org/r/49585/#comment206012>

    Already in parent pom's dependencies. So, not required



addons/falcon-bridge-shim/pom.xml (line 67)
<https://reviews.apache.org/r/49585/#comment206013>

    This is done even in falcon-bridge/pom. So, not required here?



addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java (line
31)
<https://reviews.apache.org/r/49585/#comment206019>

    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



addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java (line
46)
<https://reviews.apache.org/r/49585/#comment206020>

    We should use aspectj and remove loggers and activate and deactive. If you can't address
in this patch, can you file a bug?



addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java (line
183)
<https://reviews.apache.org/r/49585/#comment206021>

    Shouldn't Class.forName also be after activate?



addons/falcon-bridge-shim/src/main/java/org/apache/atlas/falcon/hook/AtlasService.java (line
190)
<https://reviews.apache.org/r/49585/#comment206022>

    Extra field variable is un-necessary



addons/falcon-bridge/pom.xml (line 199)
<https://reviews.apache.org/r/49585/#comment206010>

    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.


- Shwetha GS


On July 4, 2016, 8:33 a.m., Madhan Neethiraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49585/
> -----------------------------------------------------------
> 
> (Updated July 4, 2016, 8:33 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