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 59821: ATLAS-1805:Provide an Atlas hook to send Hbase Namespace/Table/column family metadata to Atlas
Date Fri, 01 Sep 2017 17:45:42 GMT

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


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