Re: Review Request 72069: ATLAS-3602: Provide option to Ignore relationship attribute mapping in getAndCacheEntity()

[Madhan Neethiraj <ma...@apache.org>]

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




repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java
Lines 80 (patched)
<https://reviews.apache.org/r/72069/#comment307637>

    - 'applicationProperties' seems to be referenced only in the constructor. If true, this doesn't have to be an instance member. Please review.
    - consider replacing ApplicationProperties.get() with a constructor parameter "Configuration config" (i.e. use DI) - see DiscoveryREST.java for such usage



repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java
Lines 81 (patched)
<https://reviews.apache.org/r/72069/#comment307638>

    having 'ignoreRelationshipAttributes' as an instance member of AtlasInstanceConverter will cause all use of AtlasInstanceConverter to ignore relationship-attributes. Instead, consider localizing this to getAndCacheEntity(), with a parameter to this method and have the caller pass appropriate flag (from notification/audit modules).
      
      public AtlasEntity getAndCacheEntity(String guid) {
        return getAndCacheEntity(guid, false); // current behaviour
      }
      
      public AtlasEntity getAndCacheEntity(String guid, boolean ignoreRelationshipAttributes) {
        ...
      }



repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphRetriever.java
Lines 576 (patched)
<https://reviews.apache.org/r/72069/#comment307639>

    EntityGraphRetriever already has an instance member 'ignoreRelationshipAttr'; method parameter 'ignoreRelationshipAttributes' here can be avoided by having the caller (AtlasInstanceConverter) use different instance of EntityGraphRetriever:
    
      class AtlasInstanceConverter {
        private final EntityGraphRetriever entityGraphRetriever;
        private final EntityGraphRetriever entityGraphRetrieverIgnoreRelationshipAttributes;
    
        ..
        
        public AtlasInstanceConverter(...) {
          this.entityGraphRetriever                             = new EntityGraphRetriever(typeRegistry, false);
          this.entityGraphRetrieverIgnoreRelationshipAttributes = new EntityGraphRetriever(typeRegistry, true);
          ...
        }
      }
    
    Please review.


- Madhan Neethiraj


On Jan. 31, 2020, 7:23 a.m., Sarath Subramanian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72069/
> -----------------------------------------------------------
> 
> (Updated Jan. 31, 2020, 7:23 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Mandar Ambawane, mayank jain, Nixon Rodrigues, and Sidharth Mishra.
> 
> 
> Bugs: ATLAS-3602
>     https://issues.apache.org/jira/browse/ATLAS-3602
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Provide option to Ignore relationship attribute mapping in getAndCacheEntity()
> 
> 
> Diffs
> -----
> 
>   repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 01e339d23 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphRetriever.java dc4c399ae 
> 
> 
> Diff: https://reviews.apache.org/r/72069/diff/1/
> 
> 
> Testing
> -------
> 
> Manually validated and perf improvement seen during add/delete classifications
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>