----------------------------------------------------------- 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 > >
