----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59821/#review184324 -----------------------------------------------------------
addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/bridge/HBaseAtlasHook.java Lines 87 (patched) <https://reviews.apache.org/r/59821/#comment260520> Since the hook uses Kafka notificaiton to update Atlas of HBase activities, AtlasClient should not be required. Please review and remove AtlasClient references, to avoid bringing in unused libraries in HBase hook. addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/bridge/HBaseAtlasHook.java Lines 103 (patched) <https://reviews.apache.org/r/59821/#comment260451> executor doesn't seem to be used. Please review. addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/event/HBaseEvent.java Lines 27 (patched) <https://reviews.apache.org/r/59821/#comment260516> HBaseContext seems to have all the information in HBaseEvent (operation & user). Consider removing HBaseEvent. addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java Lines 44 (patched) <https://reviews.apache.org/r/59821/#comment260511> Consider marking this as final - as the value is set only in the constructor. addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java Lines 54 (patched) <https://reviews.apache.org/r/59821/#comment260514> Following block is always followed by handleHBase*Operation() calls. It might be clearer to have this block inside handleHBase*Operation() methods. if (hbaseEvent != null) { hBaseAtlasHook.notifyAsPrivilegedAction(hbaseEvent); } addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java Lines 134 (patched) <https://reviews.apache.org/r/59821/#comment260513> OPERATION.ALTER_TABLE ==> OPERATION.ALTER_NAMESPACE ? addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java Lines 141 (patched) <https://reviews.apache.org/r/59821/#comment260512> Hbase ==> HBase addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java Lines 150 (patched) <https://reviews.apache.org/r/59821/#comment260518> this 'if' seems unnecessary. Please review. Similar ones in line #153, #218, addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java Lines 164 (patched) <https://reviews.apache.org/r/59821/#comment260515> Have this log call inside: if (LOG.isDebugEnabled()) { LOG.debug( ... ); } to avoid unnecessary overhead of hbaseEvent.toString(). Review other handleHBase*Operation() methods as well. addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java Lines 177 (patched) <https://reviews.apache.org/r/59821/#comment260519> Consider moving this logic (of retrieving values from hTableDescriptor to set other fields) to HBaseContext. addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/model/HBaseContext.java Lines 34 (patched) <https://reviews.apache.org/r/59821/#comment260509> Consider renaming HBaseContext as HBaseOperationContext addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/model/HBaseContext.java Lines 35 (patched) <https://reviews.apache.org/r/59821/#comment260510> It seems all fields would be known when an instance of HBaseContext is constructed. If yes, consider marking all as 'final' and remove set*() methods. addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/model/HBaseContext.java Lines 38 (patched) <https://reviews.apache.org/r/59821/#comment260508> Few fields seem redundant. Please review and remove fields that are not needed. nameSpace ==> namespaceDescriptor should have this info tableName ==> hTableDescriptor should have this info hColumnDescriptors, hColumnDescriptor: is one for column-family and other for column? addons/hbase-bridge/src/main/resources/atlas-hbase-import-log4j.xml Lines 56 (patched) <https://reviews.apache.org/r/59821/#comment260506> It will be useful to set default log level to info. Any specific reason to set warn here, but explicitly specify INFO for org.apache.atlas and com.thinkaurelius.titan? Given import is a standalone utility (i.e. that runs outside Atlas server), it shouldn't use any class from com.thinkaurelius.titan. addons/models/0060-hbase_model.json Lines 67 (patched) <https://reviews.apache.org/r/59821/#comment260505> - attributes added to an existing type should be marked optional - also, instead of updating model.json to add attributes, use a patch file. This will ensure that the attributes will get added in existing environments as well. The same applies for hbase_column_family as well. - Madhan Neethiraj On Aug. 31, 2017, 10:52 p.m., Ramesh Mani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59821/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2017, 10:52 p.m.) > > > Review request for atlas and Madhan Neethiraj. > > > Bugs: ATLAS-1805 > https://issues.apache.org/jira/browse/ATLAS-1805 > > > Repository: atlas > > > Description > ------- > > ATLAS-1805: Provide an Atlas hook to send Hbase Namespace/Table/column family > metadata to Atlas > > > Diffs > ----- > > addons/hbase-bridge-shim/pom.xml PRE-CREATION > > addons/hbase-bridge-shim/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java > PRE-CREATION > addons/hbase-bridge/pom.xml PRE-CREATION > > addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/bridge/HBaseAtlasHook.java > PRE-CREATION > > addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/event/HBaseEvent.java > PRE-CREATION > > addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessor.java > PRE-CREATION > > addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/hook/HBaseAtlasCoprocessorBase.java > PRE-CREATION > > addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/model/HBaseContext.java > PRE-CREATION > > addons/hbase-bridge/src/main/java/org/apache/atlas/hbase/model/HBaseDataTypes.java > PRE-CREATION > addons/hbase-bridge/src/main/resources/atlas-hbase-import-log4j.xml > PRE-CREATION > > addons/hbase-bridge/src/test/java/org/apache/atlas/hbase/HBaseAtlasHookTest.java > PRE-CREATION > addons/models/0060-hbase_model.json 3e46e06 > distro/src/main/assemblies/standalone-package.xml 215cb23 > pom.xml 8b9eee6 > > > Diff: https://reviews.apache.org/r/59821/diff/3/ > > > Testing > ------- > > * Review comments fixed with 2 exceptions whcih I have commented on > * Added an IT test > * Testing done in LOCAL VM > > > Thanks, > > Ramesh Mani > >
