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 Mon, 16 Jan 2017 02:57:52 GMT

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




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

    repositoryResolvedReferences seems to be populated with "guid" as the key. Is parameter
"entity" expected to be an entity guid?
    
    Please review.



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

    unresolvedEntityReferences ==> unresolvedIdReferences



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 29)
<https://reviews.apache.org/r/55531/#comment232952>

    consider adding "final" qualifier.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 42)
<https://reviews.apache.org/r/55531/#comment232956>

    add "@Override" annotation.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 58)
<https://reviews.apache.org/r/55531/#comment232958>

    isn't this same as the previous line?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 61)
<https://reviews.apache.org/r/55531/#comment232959>

    This would overwrite the previous edge storted in the context. Perhaps we should consider
context types for array and maps - like:
     GraphArrayMutationContext
     GraphMapMutationContext



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 75)
<https://reviews.apache.org/r/55531/#comment232960>

    Is removal of any other existing edges relevant only for reference type elements? How
about for structs (and classifications)?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 97)
<https://reviews.apache.org/r/55531/#comment232969>

    Consider replacing #97, #98 with the following:
     Collection<AtlasEdge> edgesToRemove = CollectionUtils.subtract(currentEntries,
newEntries);



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 99)
<https://reviews.apache.org/r/55531/#comment232962>

    Consider moving this to inside the 'if' block at line #102.



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

    in what cases would deleteEdgeReferences() return false? And why these edges should be
retained (i.e. treated as additionalElements)?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 112)
<https://reviews.apache.org/r/55531/#comment232963>

    Consider moving this to line #111 i.e. inside the previous 'if' block.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 115)
<https://reviews.apache.org/r/55531/#comment232964>

    Consider returning 'null' (and update the caller to handle null) or Collections.emptyList().



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/ArrayVertexMapper.java
(line 129)
<https://reviews.apache.org/r/55531/#comment232970>

    For better readability, consider changing the name (and signature) of the method to:
     - getEdgeAt(List<Object> elements, int index, AtlasArrayType arrayType)
       - newElements parameter seems to be used only in debug log



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java
(line 71)
<https://reviews.apache.org/r/55531/#comment232953>

    please add "@Override" annotation.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/GraphMutationContext.java
(line 23)
<https://reviews.apache.org/r/55531/#comment232954>

    Consider using parentType.getStructDef(), instead of using another member "structDef"
here?
    
    In fact, it might be useful to replace parentType/structDef/attributeDef/attrType with
a reference to AtlasAttribute (please see earlier comment).



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

    "currentEdge" - the attribute name doesn't sound good. Please review.


- Madhan Neethiraj


On Jan. 14, 2017, 3:57 a.m., Suma Shivaprasad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55531/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2017, 3:57 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 
>   repository/src/main/java/org/apache/atlas/RepositoryMetadataModule.java 54dda50 
>   repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java be9fed2

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

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

>   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/graph/GraphBackedMetadataRepositoryTest.java
7444bf3 
>   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/AtlasInstanceRestAdapters.java ad16be7

>   webapp/src/main/java/org/apache/atlas/web/rest/EntitiesREST.java f6acd07 
> 
> 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