----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59719/#review177479 -----------------------------------------------------------
intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java Lines 182 (patched) <https://reviews.apache.org/r/59719/#comment251054> I think it will help to keep classes in model package (i.e. the ones used in REST inteface) simple, with no validation, etc. There is not enough context here to perform validatation here - like whether the types referenced by endPoints are valid or not. And, given these validations can fail during deserialization of REST call parameters, the call might not make to Atlas application code - hence the error message will not be in logs. The validations should move to AtlasRelationshipType.resolveReferences(), where other validations like references to other types can be validated. intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java Lines 273 (patched) <https://reviews.apache.org/r/59719/#comment251055> - propagateTags needs to be copied as well - copy of attributeDefs should already be handled in super(other) above intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java Lines 279 (patched) <https://reviews.apache.org/r/59719/#comment251056> addAttribute() and setAttributeDefs() method overrides here seem unnecessary. Please review. intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java Lines 299 (patched) <https://reviews.apache.org/r/59719/#comment251057> relationshipCategory and propagateTags are missed here. Please review. intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java Lines 313 (patched) <https://reviews.apache.org/r/59719/#comment251058> propagateTags is not being handled here. Please review. intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java Lines 322 (patched) <https://reviews.apache.org/r/59719/#comment251059> relationshipCategory and propagateTags are missed here. Please review. intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndPointDef.java Lines 159 (patched) <https://reviews.apache.org/r/59719/#comment251060> "attribute" ==> "name" intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 68 (patched) <https://reviews.apache.org/r/59719/#comment251061> It seems lines #68 - #72 are not needed; these should be handled in the above call to super.resolveReferences(typeRegistry). This override may not be necessary. Please review. intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 76 (patched) <https://reviews.apache.org/r/59719/#comment251069> Given this override simply calls base class implementation, consider removing this method. intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 82 (patched) <https://reviews.apache.org/r/59719/#comment251071> This override should: - take AtlasRelationship as parameter type; if this type is not defined yet, Sarath can update this method later once his instance-API adds the type - call super.populateDefaultValues(obj) to handle struct attribute default values intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 100 (patched) <https://reviews.apache.org/r/59719/#comment251072> This method is already present in the base class, AtlasStructType. Unless this override has a different implementation, consider removing this method. intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 122 (patched) <https://reviews.apache.org/r/59719/#comment251073> "AtlasEntity" ==> "AtlasRelationship" intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 141 (patched) <https://reviews.apache.org/r/59719/#comment251074> "AtlasEntity" ==> "AtlasRelationship" intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 154 (patched) <https://reviews.apache.org/r/59719/#comment251077> This override should not be necessary, as base class implementation should suffice. Please review. intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java Lines 161 (patched) <https://reviews.apache.org/r/59719/#comment251076> This method should not be overridden for AtlasRelationType. This is needed only for array/map/entity types. repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java Lines 273 (patched) <https://reviews.apache.org/r/59719/#comment251086> 'typesystem' is for legacy support and I think there shouldn't be a need to keep it updated for core model changes like introduction of relationships. It will save a lot of time if we keep these features accessible only via new V2 API. Consider reverting these changes. typesystem is currently used by DSL and notification libraries. These need to be reviewed and updated to use V2 equivalents (AtlasTypeRegistry). webapp/src/main/java/org/apache/atlas/web/rest/TypesREST.java Line 22 (original), 22 (patched) <https://reviews.apache.org/r/59719/#comment251082> Avoiding changes like 'collape of imports', 'whitespace only changes' will make reviewing a little easier and importantly will not distract from focusing on important changes. - Madhan Neethiraj On June 9, 2017, 1:22 p.m., David Radley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59719/ > ----------------------------------------------------------- > > (Updated June 9, 2017, 1:22 p.m.) > > > Review request for atlas, Graham Wallis, Madhan Neethiraj, and Sarath > Subramanian. > > > Repository: atlas > > > Description > ------- > > This patch introduces the relationshipDef as a new top level TypeDef, that is > stored as a vertex in the graph. Other subtasks are required to complete the > Relationshipdef work. > > 1) relationshipdef updates and deletes have not been sueccessfully tested > 2) further constraints are required - around checking exising EntityDefs and > RelationshipDefs for consistancy. This piece will not be handled in this > subtask > 3) Creation of edges between xxxDef vertexes. I will update the design with a > proposal > > > Diffs > ----- > > common/src/main/java/org/apache/atlas/repository/Constants.java > bcdf08cdfbf1d4d8689d3d79413b2ff181b621a4 > intg/src/main/java/org/apache/atlas/AtlasErrorCode.java > d723b2a9fe03245f78bf9af53058aaa801e62aff > intg/src/main/java/org/apache/atlas/model/TypeCategory.java > e47a8a7dab0aac6154833a58148412590be6f796 > intg/src/main/java/org/apache/atlas/model/typedef/AtlasBaseTypeDef.java > 7308eb73b513660affaf35b944556d7076289815 > intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndPointDef.java > PRE-CREATION > intg/src/main/java/org/apache/atlas/model/typedef/AtlasTypesDef.java > af95bff5b53bf14057c53820cc62255d37c50498 > intg/src/main/java/org/apache/atlas/store/AtlasTypeDefStore.java > 198bd8fe515a96e654b24de3af92b6edfac3a6ae > intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java > PRE-CREATION > intg/src/main/java/org/apache/atlas/type/AtlasTypeRegistry.java > 1b3526bfcc7d13aa397844c5dec55e34dbc8ed7e > intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java > c0135f524b2ee926fb94aae31e6b49dab424a19a > intg/src/test/java/org/apache/atlas/model/ModelTestUtil.java > 084bcc4609591fd24dc0ee79290be1b337068e6a > > intg/src/test/java/org/apache/atlas/model/typedef/TestAtlasRelationshipDef.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasRelationshipDefStore.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasRelationshipType.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java > 17b7e17742de97bb9de2a4b375fea3c58b75e26b > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipDefStoreV1.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasTypeDefGraphStoreV1.java > f0c83806980153bab8a31647281015376a9d2168 > > repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java > 7a064b600cb6b3e02a814f45370fcc9041ebcd7e > repository/src/test/scala/org/apache/atlas/query/QueryTestsUtils.scala > c844558a9463d0953274ba28c54e08272a93ce89 > typesystem/src/main/java/org/apache/atlas/typesystem/types/DataTypes.java > 21d5f1a1e7488c73ab84ec9512d488ed3b9002bf > > typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipEndPointDef.java > PRE-CREATION > > typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipTypeDefinition.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystem.java > 262f784f50805c8fccde77cfe739e8538b49ab8d > > typesystem/src/main/java/org/apache/atlas/typesystem/types/utils/TypesUtil.java > f131458fc36dd4e9c29b49ce903446526d020877 > typesystem/src/main/scala/org/apache/atlas/typesystem/TypesDef.scala > b51048df2c9bccf904ffd5287d5021d2294fe458 > > typesystem/src/main/scala/org/apache/atlas/typesystem/builders/TypesBuilder.scala > 5ea345feeee79dec15c9fa5cd27724aedf50eaae > > typesystem/src/main/scala/org/apache/atlas/typesystem/json/TypesSerialization.scala > 4478a44b55f745076c9f15a47371b3863ca56c9c > webapp/src/main/java/org/apache/atlas/web/rest/TypesREST.java > c32f36ea3a5025d2cec11b6ac0bdfe192e9c05f9 > > > Diff: https://reviews.apache.org/r/59719/diff/2/ > > > Testing > ------- > > Junits show an error. I am publishing this patch - now at master, for more > feedback. I will investigate. > 1) create relationshipDef > 2) get typedefs > 3) get typedef headers > 4) get relationshgipdef by name > 5) get relationshipDef by guid. > > There is code in for delete - but it does not appear to be effective. I am > continuing to invesigate > There is code in for update - but this is failing as the delete is not > working I am continuing to invesigate > > > Thanks, > > David Radley > >
