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 68412: ATLAS-1773: Atlas OMRS Repository Connector
Date Sun, 26 Aug 2018 17:28:01 GMT

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



Graham - here are my comments from partial review; if you would like to discuss any in further
detail, please let me knwo. I will continue reviewing.


open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java
Lines 179 (patched)
<https://reviews.apache.org/r/68412/#comment291592>

    Instead of waiting for 1 second before processing every message, it might be useful to
add a retry inside processMessage() - can result in better message processing throughput.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java
Lines 181 (patched)
<https://reviews.apache.org/r/68412/#comment291591>

    It is generally not a good idea to catch & ignore 'InterruptedException'. This can
potentially prevent the JVM from shutting down.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java
Lines 359 (patched)
<https://reviews.apache.org/r/68412/#comment291593>

    line #351 and #359 will result in retrieving the entity twice from the store; consider
avoiding this duplicate retrieval - for better performance.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java
Lines 367 (patched)
<https://reviews.apache.org/r/68412/#comment291594>

    While processing ENTITY_DELETE notificaiton, getByGuid() call on Atlas would fail if Atlas
is configured for hard-delete. Connector should be able to process this notification only
with entity-guid.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java
Lines 389 (patched)
<https://reviews.apache.org/r/68412/#comment291595>

    eventTime should come from Atlas in 'notification' - given OMRS connector might process
the message at a much later time (than when the notification was generated). Consider filing
an Atlas JIRA to add timestamp in EntityNotification.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapperProvider.java
Lines 39 (patched)
<https://reviews.apache.org/r/68412/#comment291596>

    For 'static final' members, consider using upper class names.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapperProvider.java
Lines 62 (patched)
<https://reviews.apache.org/r/68412/#comment291597>

    It seems super.connectorClassName and super.connectorTypeBean might be set only once i.e.
not mutable. If yes, consider setting this via a protected constructor:
    
    public AtlasOMRSRepositoryEventMapperProvider() {
      super(AtlasOMRSRepositoryEventMapper.class.getName(), initConnectorType());
    }
    
    private ConnectorType initConnectorType() {
      ConnectorType connectorType = new ConnectorType();
    
      connectorType.setType(ConnectorType.getConnectorTypeType());
      connectorType.setGUID(connectorTypeGUID);
      connectorType.setQualifiedName(connectorTypeName);
      connectorType.setDisplayName(connectorTypeName);
      connectorType.setDescription(connectorTypeDescription);
      connectorType.setConnectorProviderClassName(this.getClass().getName());
    
      return connectorType;
    }



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java
Lines 57 (patched)
<https://reviews.apache.org/r/68412/#comment291598>

    It seems these members are immutable; if yes, please consider marking them as 'final'.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java
Lines 60 (patched)
<https://reviews.apache.org/r/68412/#comment291603>

    Since this module runs within Atlas, it might be easier and efficient to work with AtlasTypeRegistry
- which has all types in-memory and in a more consumable way (for example, detecting whether
a type is primitive or not is as simple as:
      attributeType.getTypeCategory() == TypeCategory.PRIMITIVE;
    
    AtlasTypeDefStore would read from store, which can/should be avoided.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java
Lines 104 (patched)
<https://reviews.apache.org/r/68412/#comment291599>

    For return-type and parameter-types, consider using possible super-most type in the hieratchy;
in this case replace ArrayList with List.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java
Lines 125 (patched)
<https://reviews.apache.org/r/68412/#comment291602>

    convertAtlasAttributeDef() can return null. Should such entries be added to the list returned
from here?



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java
Lines 129 (patched)
<https://reviews.apache.org/r/68412/#comment291600>

    To help in troubleshooting, consider including the exception in the error log generated
- simply by adding the exception argument at the end of arguments list.
      LOG.error("error message {}", arg1, e);
    
    Please review rest of error logs for above.



open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java
Lines 190 (patched)
<https://reviews.apache.org/r/68412/#comment291601>

    This is comment for 'cardinality' definition in openmetada: "UNORDERED" doesn't capture
the uniqueness of the elements in a SET.
    
      - SET:  unordered, unique elements
      - LIST: ordered, can contain duplicate elements


- Madhan Neethiraj


On Aug. 17, 2018, 2:38 p.m., Graham Wallis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68412/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2018, 2:38 p.m.)
> 
> 
> Review request for atlas.
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> ATLAS-1773: Atlas OMRS Repository Connector
> 
> 
> Diffs
> -----
> 
>   open-metadata/pom.xml PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/eventmapper/AtlasOMRSRepositoryEventMapperProvider.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeDefMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasAttributeMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasBaseTypeDefMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasClassificationDefMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasEntityDefMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasEntityMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasRelationshipDefMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasRelationshipMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasStoresProxy.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/AtlasStoresProxyImpl.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/Comparator.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/DSLQueryHelper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/EntityDefMapper.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/FamousFive.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/ISpringBridge.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSErrorCode.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSMetadataCollection.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSRepositoryConnector.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/LocalAtlasOMRSRepositoryConnectorProvider.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/SpringBridge.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TypeDefsByCategory.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TypeNameUtils.java
PRE-CREATION 
>   open-metadata/src/main/java/org/apache/atlas/openmetadata/admin/server/spring/OpenMetadataAdminResource.java
PRE-CREATION 
>   open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasClassificationDefMapper.java
PRE-CREATION 
>   open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasEntityDefMapper.java
PRE-CREATION 
>   open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasEntityMapper.java
PRE-CREATION 
>   open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasRelationshipDefMapper.java
PRE-CREATION 
>   open-metadata/src/test/java/org/apache/atlas/openmetadata/adapters/repositoryconnector/TestAtlasRelationshipMapper.java
PRE-CREATION 
>   pom.xml c60a53b0e2e7038b6228e16ceba47a0080d58622 
>   webapp/pom.xml 78aa81e8046b676f47d24eff0671225a3c960929 
>   webapp/src/main/webapp/WEB-INF/openMetadataContext.xml PRE-CREATION 
>   webapp/src/main/webapp/WEB-INF/web.xml 23dc0637a8b521ab89a16ec8b03895cdaf8bc7d8 
> 
> 
> Diff: https://reviews.apache.org/r/68412/diff/1/
> 
> 
> Testing
> -------
> 
> Unit Testing (approx 80 tests) and Functional Testing of major operations (creation of
types, instances, reference copies and proxies).
> 
> 
> Thanks,
> 
> Graham Wallis
> 
>


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