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