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 55531: ATLAS-1467 Create/Full Update implementation
Date Thu, 19 Jan 2017 08:47:35 GMT

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




intg/src/main/java/org/apache/atlas/type/AtlasStructType.java (line 264)
<https://reviews.apache.org/r/55531/#comment233582>

    I think 'string' should not be a valid value for a Struct/Entity/Classification. If there
is no usecase to allow this value, please remove this.



intg/src/main/java/org/apache/atlas/type/AtlasStructType.java (line 266)
<https://reviews.apache.org/r/55531/#comment233583>

    AtlasObjectId should be allowed only when attribute-type is a entity. It wouldn't be a
valid value for struct or classficaition. Please review.



repository/src/main/java/org/apache/atlas/repository/store/graph/EntityGraphDiscoveryContext.java
(line 111)
<https://reviews.apache.org/r/55531/#comment233589>

    unresolvedEntityReferences is type List<AtlasEntity>. would calling remove(AtlasObjectId
id) result in correct removal? Please review.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/StructVertexMapper.java
(line 56)
<https://reviews.apache.org/r/55531/#comment233602>

    Consider marking graph, mapVertexMapper and arrVertexMapper as final.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/StructVertexMapper.java
(line 157)
<https://reviews.apache.org/r/55531/#comment233603>

    Should vertex be cleaned of properties that are not present in value?
    
    updateTime, updatedBy system attributes?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/StructVertexMapper.java
(line 174)
<https://reviews.apache.org/r/55531/#comment233604>

    Shouldn't createdBy and updatedBy attributes be set here?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/UniqAttrBasedEntityResolver.java
(line 46)
<https://reviews.apache.org/r/55531/#comment233596>

    Consider marking 'typeRegistry' and 'graphHelper' as final.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/UniqAttrBasedEntityResolver.java
(line 55)
<https://reviews.apache.org/r/55531/#comment233598>

    Consider moving this to line #49, along with other attributes.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/UniqAttrBasedEntityResolver.java
(line 93)
<https://reviews.apache.org/r/55531/#comment233599>

    after the validation in line #83, there shouldn't be any unresolvedEntityReference here,
right?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/UniqAttrBasedEntityResolver.java
(line 107)
<https://reviews.apache.org/r/55531/#comment233600>

    String qualifiedAttrName = attr.getQualifiedName();



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/UniqAttrBasedEntityResolver.java
(line 114)
<https://reviews.apache.org/r/55531/#comment233601>

    Please add if (LOG.isDebugEnabled())



server-api/src/main/java/org/apache/atlas/RequestContextV1.java (line 47)
<https://reviews.apache.org/r/55531/#comment233591>

    Consider setting this to final and initialize in the constructor.



server-api/src/main/java/org/apache/atlas/RequestContextV1.java (line 59)
<https://reviews.apache.org/r/55531/#comment233590>

    since CURRENT_CONTEXT is a ThreadLocal<>, synchronized() usage is not necessary.
Please review.
    
    A simpler impl could be:
    
    public static RequestContextV1 get() {
      RequestContextV1 ret = CURRENT_CONTEXT.get();
    
      if (ret == null) {
        ret = new RequestContextV1();
        CURRENT_CONTEXT.set(ret);
      }
      
      return ret;
    }



server-api/src/main/java/org/apache/atlas/RequestContextV1.java (line 106)
<https://reviews.apache.org/r/55531/#comment233594>

    Any reason to create a new list here, instead of simply returning 'createdEntityIds'?
The return can be changed to Collection<String>. Please review other methods here for
this suggestion.


- Madhan Neethiraj


On Jan. 19, 2017, 6:22 a.m., Suma Shivaprasad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55531/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2017, 6:22 a.m.)
> 
> 
> Review request for atlas.
> 
> 
> Bugs: ATLAS-1467
>     https://issues.apache.org/jira/browse/ATLAS-1467
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Implemented
> 
> 1. Entity graph discovery - This differs from current implementation in that it collects
all references first and resolves them later . The current implemenation tries to resolve
references at the same time resulting in issues when entities have cross references to each
other . Also it handles resolving a refernce by its unique attribute  (by supplying an AtlasEntity
instead of an Id)
> 2. Create with new APIs and POJOs   ( no change in graph model )
> 3. Full Update with new APIs and POJOs  ( no change in graph model )
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 6770c41 
>   intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java 2ad0f76 
>   intg/src/main/java/org/apache/atlas/model/instance/AtlasEntityHeader.java e7b70aa 
>   intg/src/main/java/org/apache/atlas/model/instance/EntityMutations.java 3501c90 
>   intg/src/main/java/org/apache/atlas/model/typedef/AtlasEntityDef.java 85e9d77 
>   intg/src/main/java/org/apache/atlas/model/typedef/AtlasStructDef.java aee26ef 
>   intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java 8772720 
>   intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java 3625f72 
>   intg/src/main/java/org/apache/atlas/type/AtlasStructType.java e20af76 
>   intg/src/test/java/org/apache/atlas/TestUtilsV2.java 53b109c 
>   intg/src/test/java/org/apache/atlas/type/TestAtlasTypeRegistry.java 9429c07 
>   repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java 54dda50 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java 2be9a2d

>   repository/src/main/java/org/apache/atlas/discovery/EntityLineageService.java 45e2dd2

>   repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java 5259249

>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java
f17b816 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/EntityGraphDiscovery.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/EntityGraphDiscoveryContext.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/EntityResolver.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityGraphDiscoveryV1.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
e731c11 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEnumDefStoreV1.java
fccbeba 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java
18b3b85 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasStructDefStoreV1.java
e780dc1 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/GraphMutationContext.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/HardDeleteHandlerV1.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/IDBasedEntityResolver.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/InstanceGraphMapper.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/MapVertexMapper.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/SoftDeleteHandlerV1.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/StructVertexMapper.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/UniqAttrBasedEntityResolver.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/util/AtlasRepositoryConfiguration.java ea0e670

>   repository/src/test/java/org/apache/atlas/lineage/EntityLineageServiceTest.java b1dac9d

>   repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
c7c3286 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java
PRE-CREATION 
>   server-api/src/main/java/org/apache/atlas/RequestContextV1.java PRE-CREATION 
>   webapp/src/main/java/org/apache/atlas/web/adapters/AtlasEntityFormatConverter.java
c4be236 
>   webapp/src/main/java/org/apache/atlas/web/adapters/AtlasInstanceRestAdapters.java ad16be7

>   webapp/src/main/java/org/apache/atlas/web/adapters/AtlasStructFormatConverter.java
3565ab3 
>   webapp/src/main/java/org/apache/atlas/web/rest/EntitiesREST.java f6acd07 
>   webapp/src/main/java/org/apache/atlas/web/util/LineageUtils.java 54ca236 
>   webapp/src/test/java/org/apache/atlas/web/adapters/TestEntitiesREST.java 265b650 
>   webapp/src/test/java/org/apache/atlas/web/adapters/TestEntityREST.java 2a75773 
>   webapp/src/test/java/org/apache/atlas/web/resources/BaseResourceIT.java dcb1264 
>   webapp/src/test/java/org/apache/atlas/web/resources/EntityDiscoveryJerseyResourceIT.java
2ade5b0 
>   webapp/src/test/java/org/apache/atlas/web/resources/EntityV2JerseyResourceIT.java d7702e2

> 
> Diff: https://reviews.apache.org/r/55531/diff/
> 
> 
> Testing
> -------
> 
> UTs for create
> 
> TODO:
> Update testing pending
> Error handling for unresolved references
> UTS for discovery
> 
> 
> Thanks,
> 
> Suma Shivaprasad
> 
>


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