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 61526: ATLAS-2029: Restrict entities, classifications can be applied to
Date Sun, 20 Aug 2017 18:56:41 GMT

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




intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 54 (patched)
<https://reviews.apache.org/r/61526/#comment259322>

    >> we need to store the entityTypes specified in our supertypes. i.e. our parent
classificationDefs may specify more entityTypes that we also need to allow
        
    This comment doesn't look correct. A classification can have restricted entity-types than
its super-types.



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 110 (patched)
<https://reviews.apache.org/r/61526/#comment259323>

    >> If our classificationDef or any of our parent ClassificaitonDefs have an empty
list of entityTypes, then we will not restrict the entities applied to this Classification.
    
    This doesn't look correct. It should be possible for a classtification to be applicable
to any entity-type but its sub-type classifications to allow only few entity-types.
    
    Here, we should ensure that 'entityTypes' for this classificationDef (if specified) doesn't
contain any entityType that is not applicable for superType classifications.



intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java
Lines 386 (patched)
<https://reviews.apache.org/r/61526/#comment259324>

    blocks between #387 and #392 can be folded into a the following statement. Please review:
    
      boolean canApply = entityTypes.isEmpty() || entityTypes.contains(entity.getTypeName())
|| CollectionUtils.containsAny(entityTypes.entityType.getAllSuperTypes());



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
Lines 728 (patched)
<https://reviews.apache.org/r/61526/#comment259325>

    Once again, I am not sure why you would retrieve "AtlasEntityWithExtInfo" here - as the
only information needed here seem to be typeName of the entity with the given guid. Populating
AtlasEntityWithExtInfo would require reading all properties of the entity *and* properites
of all entities referenced by this entity. For example, if table has 100s of columns, getById()
would read vertex for the table and vertices for each column in the table. Since all these
details are not necessary here, I suggested earlier to retrieve only typeName for the entity.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
Lines 742 (patched)
<https://reviews.apache.org/r/61526/#comment259326>

    Ensure that references are resolved when typeRegistry updates are made. Whenever *NoRefResolve()
methods are used to update the registry, ensure to run typeRegistry.resolveReferences() at
the end of updates. You can see references in number of methods in AtlasTransientTypeRegistry
- like addTypes(), updateTypes().
    
    Calling resolveReferences() outside of AtlasTransientTypeRegistry is asking for trouble,
as the would modify the type instance which might be used in other threads.
    
    @Sarath - it might be worth removing "public" access to AtlasType.resolveReferences()
to avoid possible misuse like here.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
Lines 429 (patched)
<https://reviews.apache.org/r/61526/#comment259328>

    It will help readability a lot if a space can be added after a comma. Please review rest
of the patch for this and update. I find having a readable code to be one of the cheapest
way to spot potential issues.



repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java
Line 944 (original), 944 (patched)
<https://reviews.apache.org/r/61526/#comment259327>

    For better readability:
      "+e.getMessage()" ==> "+ e.getMessage()"


- Madhan Neethiraj


On Aug. 20, 2017, 11:34 a.m., David Radley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61526/
> -----------------------------------------------------------
> 
> (Updated Aug. 20, 2017, 11:34 a.m.)
> 
> 
> Review request for atlas, Graham Wallis, Madhan Neethiraj, and Sarath Subramanian.
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> ATLAS-2029: Restrict entities, classifications can be applied to
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 2503d8ef203cf4efbe15b440257b1da2252b6153

>   intg/src/main/java/org/apache/atlas/model/typedef/AtlasClassificationDef.java eeaf71413a56c08db8170fd3323b8e8245ae44fe

>   intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java 56c3ed38392d2d2c8882861373cd42a549b5670d

>   intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java 427439ca97d496f02de2d38329c582ded239c04c

>   intg/src/test/java/org/apache/atlas/TestUtilsV2.java fc65af057255b4c17378080ee4fb7cbfc780c3fe

>   intg/src/test/java/org/apache/atlas/type/TestAtlasClassificationType.java aaf4a6ac0a978e5eb6de41279cae1b1c82373374

>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasClassificationDefStoreV1.java
89445048c69555b86a5347b21dde5a21dae9b2a1 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java
1c168b4cff0d105c7a0d4a9fbdb50871388c917e 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java
227f7cd12a9b23c3bbc1cfdc40d06616ea775ca4 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java
50a421662b9a58ea3daf45e57d21626b5f2c6c44 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStoreTest.java
8638a7f17e8a17d3a6e0bfb94879b5c5406be1a3 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java
62fad5b02a7229d9bc3083690980eb063756bc29 
> 
> 
> Diff: https://reviews.apache.org/r/61526/diff/2/
> 
> 
> Testing
> -------
> 
> Testing
> performed unit tests
> 
> Using postman
> 1) create an entityDef  aaa
> 2) create a classificationDef with an entitytype  aaaa - checked that it is in the response
> 3) Create a entity instance of aaa
> 4) add the classification to it
> 5) Create an entity instance with a different type bbbb
> 6) Attempt to add the classification to bbbb. this fails with an informative message
> 7) Attempt to update the ClassificationDef to remove the entity type - this fails with
an informative message
> 8) Attempt to update the classificationdef to add bbbb. this update works. 
> 9) Attempt to add an entity type that does not exist to the ClassificationDef. this should
fail.  
> 10) Attempt to update an entity type that does not exist to the ClassificationDef. this
should fail.
> 
> 
> Thanks,
> 
> David Radley
> 
>


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