atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Radley <david_rad...@uk.ibm.com>
Subject Re: Review Request 59769: [ATLAS-1856]: Relationship instance API Java & REST implementation
Date Tue, 20 Jun 2017 09:51:03 GMT

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




authorization/src/main/java/org/apache/atlas/authorize/AtlasResourceTypes.java
Line 22 (original), 22 (patched)
<https://reviews.apache.org/r/59769/#comment252264>

    This should be RELATIONSHIP



authorization/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java
Lines 142 (patched)
<https://reviews.apache.org/r/59769/#comment252265>

    This should be relationship



authorization/src/main/java/org/apache/atlas/authorize/simple/PolicyParser.java
Lines 234 (patched)
<https://reviews.apache.org/r/59769/#comment252266>

    same



graphdb/titan1/src/main/java/org/apache/atlas/repository/graphdb/titan1/Titan1Graph.java
Line 408 (original), 403 (patched)
<https://reviews.apache.org/r/59769/#comment252270>

    I am wondering what the intent is here- please could you add a comment?



intg/src/main/java/org/apache/atlas/model/instance/AtlasRelationship.java
Lines 227 (patched)
<https://reviews.apache.org/r/59769/#comment252271>

    Should relationship equals only check the structural parts. Not createdBy , updatetime
etc.



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndDef.java
Line 145 (original), 169 (patched)
<https://reviews.apache.org/r/59769/#comment252272>

    In the RelationshipDef review - I was advised to use getIsContainter in case the json
aparsing needs this.



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndDef.java
Line 147 (original), 171 (patched)
<https://reviews.apache.org/r/59769/#comment252273>

    setContainer -> setIsContainer



intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java
Lines 131 (patched)
<https://reviews.apache.org/r/59769/#comment252274>

    the method name is isTypeOrSuperTypeOf, but the check is of subtypes. Is this what you
intended?



intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java
Lines 60 (patched)
<https://reviews.apache.org/r/59769/#comment252275>

    can relationshipDef be null here?



intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java
Lines 61 (patched)
<https://reviews.apache.org/r/59769/#comment252276>

    if we need to check relationshipDef not null- I suggest we do it only once.



intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java
Lines 63 (patched)
<https://reviews.apache.org/r/59769/#comment252278>

    if an end type is null -shouldn't we stop any firther processing in this method.



intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java
Lines 66 (patched)
<https://reviews.apache.org/r/59769/#comment252277>

    Rather than use reflection I suggest checking the category using getTypeCategory.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
Lines 101 (patched)
<https://reviews.apache.org/r/59769/#comment252279>

    So we create the first edge of the relationship and then additional legacy edges. Will
any legacy implmnetations be effected by the new edge?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
Lines 106 (patched)
<https://reviews.apache.org/r/59769/#comment252280>

    I am not sure this is right. It is valid to have more than one relationship between 2
entities.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
Lines 185 (patched)
<https://reviews.apache.org/r/59769/#comment252281>

    unknown relationship ==> unknown relationshiptype



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
Lines 197 (patched)
<https://reviews.apache.org/r/59769/#comment252282>

    in the Def code there is a get method that then works out if it needs to do a getbyName
or get bu Guid. I suggest following the same pattern.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
Lines 222 (patched)
<https://reviews.apache.org/r/59769/#comment252283>

    I assume we need to validate the attribute name as well as the AtlasObjectId content.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
Lines 264 (patched)
<https://reviews.apache.org/r/59769/#comment252284>

    I suggest the default version be a constant.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
Lines 301 (patched)
<https://reviews.apache.org/r/59769/#comment252285>

    where do we add the relationship attributes ?


- David Radley


On June 20, 2017, 1:55 a.m., Sarath Subramanian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59769/
> -----------------------------------------------------------
> 
> (Updated June 20, 2017, 1:55 a.m.)
> 
> 
> Review request for atlas, Apoorv Naik, Ashutosh Mestry, Madhan Neethiraj, Nixon Rodrigues,
and Suma Shivaprasad.
> 
> 
> Bugs: ATLAS-1856
>     https://issues.apache.org/jira/browse/ATLAS-1856
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Based on the relationDef implementation in ATLAS-1852. Implement instance API and REST
for entity relationships. This patch is not complete.
> work in progress
> 
> 
> Diffs
> -----
> 
>   authorization/src/main/java/org/apache/atlas/authorize/AtlasResourceTypes.java deccf843

>   authorization/src/main/java/org/apache/atlas/authorize/simple/AtlasAuthorizationUtils.java
93d988e6 
>   authorization/src/main/java/org/apache/atlas/authorize/simple/PolicyParser.java 7ef49e66

>   graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasGraphQuery.java
bd7b35e4 
>   graphdb/common/src/main/java/org/apache/atlas/repository/graphdb/titan/query/NativeTitanGraphQuery.java
662a2705 
>   graphdb/common/src/main/java/org/apache/atlas/repository/graphdb/titan/query/TitanGraphQuery.java
056088cf 
>   graphdb/titan0/src/main/java/org/apache/atlas/repository/graphdb/titan0/query/NativeTitan0GraphQuery.java
5ad176b5 
>   graphdb/titan1/src/main/java/org/apache/atlas/repository/graphdb/titan1/Titan1Graph.java
e829d918 
>   graphdb/titan1/src/main/java/org/apache/atlas/repository/graphdb/titan1/query/NativeTitan1GraphQuery.java
9dc175b6 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 6c33f402 
>   intg/src/main/java/org/apache/atlas/model/instance/AtlasRelationship.java PRE-CREATION

>   intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndDef.java 000d747b

>   intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java 0ff15827 
>   intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java eb2fc48b 
>   intg/src/main/java/org/apache/atlas/type/AtlasTypeRegistry.java aebd4d1a 
>   repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca7fad06

>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasRelationshipStore.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java
560b3385 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java
66f20daa 
>   repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java
7a064b60 
>   repository/src/test/java/org/apache/atlas/TestModules.java 095af417 
>   webapp/src/main/java/org/apache/atlas/web/rest/RelationshipREST.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59769/diff/6/
> 
> 
> Testing
> -------
> 
> Tested using POSTMAN. UTs and ITs in progress.
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>


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