----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56535/#review165581 -----------------------------------------------------------
1) The description of this request and the corresponding JIRA says "Add tests for DeleteHandlerV1", but really it seems like these changes actually involved completing the delete operation implemention for the new code path, not just adding tests. 2) There is an inconsistent naming convention for the new code path with regard to using V1 or V2. Perhaps it is beyond the scope of this JIRA to address this, but there needs to be consistent naming. The following classes use V2 in the name: ./client/src/main/java/org/apache/atlas/AtlasDiscoveryClientV2.java ./client/src/main/java/org/apache/atlas/AtlasEntitiesClientV2.java ./client/src/main/java/org/apache/atlas/AtlasLineageClientV2.java ./client/src/main/java/org/apache/atlas/AtlasTypedefClientV2.java ./intg/src/test/java/org/apache/atlas/TestUtilsV2.java ./webapp/src/main/java/org/apache/atlas/examples/QuickStartV2.java ./webapp/src/test/java/org/apache/atlas/examples/QuickStartV2IT.java ./webapp/src/test/java/org/apache/atlas/web/resources/EntityV2JerseyResourceIT.java The following classes use V1 in the name: ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasAbstractDefStoreV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasClassificationDefStoreV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityDefStoreV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityGraphDiscoveryV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEnumDefStoreV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasStructDefStoreV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/HardDeleteHandlerV1.java ./repository/src/main/java/org/apache/atlas/repository/store/graph/v1/SoftDeleteHandlerV1.java ./repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java ./server-api/src/main/java/org/apache/atlas/RequestContextV1.java repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java (lines 175 - 209) <https://reviews.apache.org/r/56535/#comment237437> This duplicates code in deleteByIds() - instead, it should simply delegate to deleteByIds() after the isEmpty() check e.g. return deleteByIds(Collections.singletonList(guid)); repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java (line 216) <https://reviews.apache.org/r/56535/#comment237438> Seems like a different (perhaps new) error code is needed here, as INSTANCE_GUID_NOT_FOUND implies the specified guid does not exist in the repository, but really this is an invalid value of the guids argument and should be noted as such. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java (line 246) <https://reviews.apache.org/r/56535/#comment237439> Rename updateReverseAttribute to updateInverseAttribute, to be consistent with the V2 naming convention of using inverse rather than reverse. repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasDeleteHandlerV1Test.java (line 301) <https://reviews.apache.org/r/56535/#comment237445> Is this comment still relevant? repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasDeleteHandlerV1Test.java (lines 357 - 358) <https://reviews.apache.org/r/56535/#comment237446> Remove commented-out code repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasDeleteHandlerV1Test.java (line 454) <https://reviews.apache.org/r/56535/#comment237440> appears to be stray comment repository/src/test/java/org/apache/atlas/repository/store/graph/v1/SoftDeleteHandlerV1Test.java (line 46) <https://reviews.apache.org/r/56535/#comment237441> HardDeleteHandlerV1Test is needed. repository/src/test/java/org/apache/atlas/repository/store/graph/v1/SoftDeleteHandlerV1Test.java (line 69) <https://reviews.apache.org/r/56535/#comment237447> There are several comments like this. I see elsewhere that entityStore.getById() is being called. So are these comments obsolete, and the calls to metadataService.getEntityDefinition() be replaced with entityStore.getById()? server-api/src/main/java/org/apache/atlas/RequestContextV1.java (line 117) <https://reviews.apache.org/r/56535/#comment237448> Remove extra whitespace - David Kantor On Feb. 14, 2017, 10:34 p.m., Suma Shivaprasad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56535/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2017, 10:34 p.m.) > > > Review request for atlas. > > > Bugs: ATLAS-1547 > https://issues.apache.org/jira/browse/ATLAS-1547 > > > Repository: atlas > > > Description > ------- > > Added UTs for delete operations > > > Diffs > ----- > > addons/models/0030-hive_model.json 33ba156 > intg/src/main/java/org/apache/atlas/model/instance/AtlasEntityHeader.java > 93a77e0 > intg/src/main/java/org/apache/atlas/model/instance/AtlasStruct.java 4e3c795 > > intg/src/main/java/org/apache/atlas/model/instance/EntityMutationResponse.java > 2f2d44f > intg/src/main/java/org/apache/atlas/model/typedef/AtlasStructDef.java > 2abc30b > intg/src/test/java/org/apache/atlas/TestUtilsV2.java f268e48 > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > 69b22ff > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java > c6a7206 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java > 61adf2b > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > 072d10d > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/HardDeleteHandlerV1.java > fe0db39 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/SoftDeleteHandlerV1.java > 7e3068b > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasDeleteHandlerV1Test.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java > bb7de4a > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/SoftDeleteHandlerV1Test.java > PRE-CREATION > server-api/src/main/java/org/apache/atlas/RequestContextV1.java 23eb4ce > > Diff: https://reviews.apache.org/r/56535/diff/ > > > Testing > ------- > > Need to fix a test failure in > AtlasDeleteHandlerV1Test.testUpdateEntity_MultiplicityOneNonCompositeReference > > > Thanks, > > Suma Shivaprasad > >
