atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Wallis <graham_wal...@uk.ibm.com>
Subject Re: Review Request 59719: ATLAS-1852 Create RelationshipDef
Date Mon, 05 Jun 2017 13:11:57 GMT

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




common/src/main/java/org/apache/atlas/repository/Constants.java
Lines 51 (patched)
<https://reviews.apache.org/r/59719/#comment250444>

    It wasn't obvious to me that these constants are used - the actual relationshipDef seems
to not use the xxx_KEY forms.



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java
Lines 49 (patched)
<https://reviews.apache.org/r/59719/#comment250450>

    Possibly nicer to import java.io.Serializable and then just refer to the class here.



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java
Lines 124 (patched)
<https://reviews.apache.org/r/59719/#comment250445>

    I think you might have wanted to test endPointDef2 here



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java
Lines 160 (patched)
<https://reviews.apache.org/r/59719/#comment250446>

    Is there no need to clone attributeDefs of 'other'?



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java
Lines 187 (patched)
<https://reviews.apache.org/r/59719/#comment250447>

    Helpful to include attributeDefs here?



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java
Lines 216 (patched)
<https://reviews.apache.org/r/59719/#comment250448>

    Remember to check attributeDefs



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java
Lines 221 (patched)
<https://reviews.apache.org/r/59719/#comment250449>

    Shouldn't the hashcode depend on the relationshipCategory as well? Ditto attributeDefs?



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndPointDef.java
Lines 70 (patched)
<https://reviews.apache.org/r/59719/#comment250451>

    Do you need this setter in addition to the setContainer() method?



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndPointDef.java
Lines 74 (patched)
<https://reviews.apache.org/r/59719/#comment250452>

    Do you need this getter in addition to the isContainer() method?



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndPointDef.java
Lines 117 (patched)
<https://reviews.apache.org/r/59719/#comment250453>

    I think you might have wanted to return this.isContainer ;-) [ No parameter needed ]



intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipEndPointDef.java
Lines 127 (patched)
<https://reviews.apache.org/r/59719/#comment250454>

    Did you really mean to label the name as attribute?



intg/src/main/java/org/apache/atlas/type/AtlasTypeRegistry.java
Line 35 (original), 49 (patched)
<https://reviews.apache.org/r/59719/#comment250456>

    importing jersey from com.sun.xxx suggests that we have mixed versions of Glassfish in
Atlas. Do we need to refine these onto a 2.x base?



intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java
Lines 273 (patched)
<https://reviews.apache.org/r/59719/#comment250458>

    Did you really want both this getTypesDef() and the one without the relationships (see
below)?



intg/src/test/java/org/apache/atlas/model/ModelTestUtil.java
Lines 456 (patched)
<https://reviews.apache.org/r/59719/#comment250459>

    Remember the TODO



repository/src/main/java/org/apache/atlas/repository/typestore/StoreBackedTypeCache.java
Lines 185 (patched)
<https://reviews.apache.org/r/59719/#comment250460>

    Any point just rethrowing existing exception (without logging etc)?



typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipEndPointDef.java
Lines 106 (patched)
<https://reviews.apache.org/r/59719/#comment250462>

    setContainer and isContainer are additional to existing setter and getter above



typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipEndPointDef.java
Lines 111 (patched)
<https://reviews.apache.org/r/59719/#comment250461>

    No parameter needed; ensure return this.isContainer.



typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipEndPointDef.java
Lines 122 (patched)
<https://reviews.apache.org/r/59719/#comment250463>

    Possibly meant to label as 'name'



typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipTypeDefinition.java
Lines 231 (patched)
<https://reviews.apache.org/r/59719/#comment250464>

    Comments appears to be out of place



typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipTypeDefinition.java
Lines 238 (patched)
<https://reviews.apache.org/r/59719/#comment250465>

    Do we need to dump more into the toString - e.g. atrributeDefs and maybe name, relationshipCategory,
etc?



typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipTypeDefinition.java
Lines 256 (patched)
<https://reviews.apache.org/r/59719/#comment250466>

    This class doesn't appear to extend anything - so would not have a super.



typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipTypeDefinition.java
Lines 270 (patched)
<https://reviews.apache.org/r/59719/#comment250467>

    No super


- Graham Wallis


On June 2, 2017, 10:25 a.m., David Radley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59719/
> -----------------------------------------------------------
> 
> (Updated June 2, 2017, 10:25 a.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 storted
as a vertex in the graph. Other subtasks with complete the Relationshipdef work. 
> 
> Work that is not included in this patch:
> 1) get relationship by guid is not working
> 2) relationshipdef updates have not been tested
> 3) further constraints are required. I will update the design doc with a proposal
> 4) Tag propagation - I think there are still ongoing discussions we should come to consensus
on this - it would be easy to add
> 5) Creation of edges between xxxDef vertexes. I will update the design with a proposal
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/atlas/ApplicationProperties.java a35bdfee72a460b3aa793a40c41618dba31e1fb9

>   common/src/main/java/org/apache/atlas/repository/Constants.java bcdf08cdfbf1d4d8689d3d79413b2ff181b621a4

>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 298df6b0171b9b5e0683cd0787c6cdfd2e714f5e

>   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/AtlasType.java 28d0a0748e3357a275bfec5b88cb522f9edfd9c2

>   intg/src/main/java/org/apache/atlas/type/AtlasTypeRegistry.java 29ea60304826ab1c94b5f74614350732415da8a6

>   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/TestAtlasClassificationDef.java a1abc6e754ff5b45e3f2aabfcc94ff356ff16327

>   intg/src/test/java/org/apache/atlas/model/typedef/TestAtlasEntityDef.java 46d4d065f6f0f89e98a7c6f56c6a261c575bbfc8

>   intg/src/test/java/org/apache/atlas/model/typedef/TestAtlasRelationshipDef.java PRE-CREATION

>   repository/src/main/java/org/apache/atlas/repository/converters/TypeConverterUtil.java
7902100e80a400a5097554800ac2d862b9148144 
>   repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java ca7fad068769dc72d90b18f7ae6ae84143792760

>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasRelationshipDefStore.java
PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasTypeDefGraphStore.java
4a8e1de7942156f0888bacb03479e7f9b2055512 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStream.java
eb860ff2a250d122881dd54eb3d2de2986f6f8e6 
>   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
3bf318f025f1b1edc9c35be08083fc8f0ed2add3 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityStream.java
3444bfd6d8b972b6f2f652a6a8e49cb0619145b6 
>   repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java
ac135863e82a815a580a095d2942088f571e35a9 
>   repository/src/main/java/org/apache/atlas/repository/typestore/ITypeStore.java 84779f409b47b29f21accb2b5a94e068801a740f

>   repository/src/main/java/org/apache/atlas/repository/typestore/StoreBackedTypeCache.java
8573719c912cfac35759da96bf4bcf8a06a75ee1 
>   repository/src/main/java/org/apache/atlas/repository/typestore/TypePersistenceVisitor.java
bfb1bfc75c1c3ab5f80937d40c325e1b065265a3 
>   repository/src/main/java/org/apache/atlas/repository/typestore/TypeVertexFinder.java
8b381524055aefcb0c7b0f07f773a6250fedb51d 
>   repository/src/main/java/org/apache/atlas/repository/typestore/TypeVisitor.java a6e353c18d690201818c899666f4fecb63172e42

>   repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java fd85ef0f6d4859ae6da53e5cac49b577e225519a

>   repository/src/test/java/org/apache/atlas/BaseRepositoryTest.java a7cb2e5b541db27bd19818ee5e3639ebca0a6389

>   repository/src/test/java/org/apache/atlas/TestUtils.java cf39d8d8130a97e5709036c1d406227f4f026027

>   repository/src/test/java/org/apache/atlas/repository/graph/GraphBackedMetadataRepositoryDeleteTestBase.java
4fd416c70bd307a9c2cc358d5a059bab1bb9d3f9 
>   repository/src/test/java/org/apache/atlas/repository/graph/ReverseReferenceUpdateTestBase.java
c08fbc99908f2a4c4be2eee337e6ef0c7151033d 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasDeleteHandlerV1Test.java
9a11e087515909e271301f85786f1bdfbdad789a 
>   repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1Test.java
1dd727661ca8a8d59caaee0f3aa9854fb4b631ea 
>   repository/src/test/java/org/apache/atlas/repository/typestore/GraphBackedTypeStoreTest.java
65cd9386ada156e5fa975433a5c5fa89698e49e0 
>   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/HierarchicalTypeDefinition.java
ab63fea3c6695065943a7b11fdb71cec8f79565d 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipEndPointDef.java
PRE-CREATION 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/RelationshipType.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 c853ec655676eed99d93ced9f0f0de18c77bc27a

>   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 
>   typesystem/src/test/java/org/apache/atlas/typesystem/json/SerializationJavaTest.java
5ee019cba021e3e372c555558ad46690ffe73ea7 
>   typesystem/src/test/java/org/apache/atlas/typesystem/types/BaseTest.java 95c99e4f6c112d71fafd11bda4c41e9dd5ce71dd

>   typesystem/src/test/java/org/apache/atlas/typesystem/types/ClassTest.java daecdd78c952747964885d4cb8cdfb59727ffee5

>   typesystem/src/test/java/org/apache/atlas/typesystem/types/EnumTest.java 23071922a4b59b690f8c670a043203a031523c28

>   typesystem/src/test/java/org/apache/atlas/typesystem/types/FieldMappingTest.java 0259ade905929aa021c11dba795f252ee07e27d8

>   typesystem/src/test/java/org/apache/atlas/typesystem/types/StructTest.java 3a1675e1a34683fc9645e3ad7a4b13a9da924aae

>   typesystem/src/test/java/org/apache/atlas/typesystem/types/TraitTest.java 7c3921386c8ad582a594504b9b0f82c513cabd40

>   typesystem/src/test/java/org/apache/atlas/typesystem/types/TypeSystemTest.java 0ef5d10d9dccae64f644470070ed5a345d191ebd

>   typesystem/src/test/scala/org/apache/atlas/typesystem/json/SerializationTest.scala
931773dd2b184181743f56971d38a09be8b96e26 
>   typesystem/src/test/scala/org/apache/atlas/typesystem/json/TypesSerializationTest.scala
cfd4bdb26482c187c4e80f110b7349df8b524e6e 
>   webapp/src/main/java/org/apache/atlas/examples/QuickStart.java 91ba1113b706415d236a3319291500c32322d83e

>   webapp/src/main/java/org/apache/atlas/examples/QuickStartV2.java a95fac37546867cfe6c43b72804c0384b091e195

>   webapp/src/main/java/org/apache/atlas/web/dao/UserDao.java b461a6a5c652d961931ed9b3dac598a5d27c30b5

>   webapp/src/main/java/org/apache/atlas/web/rest/TypesREST.java 5d846cf317ae46fa88ef7877baad129638e38e03

>   webapp/src/test/java/org/apache/atlas/util/RestUtilsTest.java 87259df2443d858dd305e71be00cee2c7e7c0815

>   webapp/src/test/java/org/apache/atlas/web/adapters/TestEntityREST.java cadf0ffffc387cf60ded99945cb90d13c86cf3d9

>   webapp/src/test/java/org/apache/atlas/web/resources/BaseResourceIT.java 457dc394680ae8e6a44fcb7619e6df7c746deff8

>   webapp/src/test/java/org/apache/atlas/web/resources/DataSetLineageJerseyResourceIT.java
ee4057d41174765e666eff0e3aaaf00ffc780e92 
>   webapp/src/test/java/org/apache/atlas/web/resources/EntityDiscoveryJerseyResourceIT.java
a51f3716d66485941c29d57f5aafdf44753176b7 
>   webapp/src/test/java/org/apache/atlas/web/resources/EntityJerseyResourceIT.java f2af20825cd04f07cf1a50f453f82dfed7880a79

>   webapp/src/test/java/org/apache/atlas/web/resources/EntityResourceTest.java 21060d14ac82e037466d8a3490aac9ba721ac611

>   webapp/src/test/java/org/apache/atlas/web/resources/EntityV2JerseyResourceIT.java 9d5ff5a829241299df84fa429d6199fc3ea16302

>   webapp/src/test/java/org/apache/atlas/web/resources/MetadataDiscoveryJerseyResourceIT.java
87d8719fda4072ef84072a7ab931fe010f9fb8cb 
>   webapp/src/test/java/org/apache/atlas/web/resources/TypesJerseyResourceIT.java 2ef33c3ff0c25a179bc7f5ca1b89c7ac12e94b7c

> 
> 
> Diff: https://reviews.apache.org/r/59719/diff/1/
> 
> 
> Testing
> -------
> 
> Junits run successfuly. 
> 1) Using the sample json, issued a Rest call to create the relationshipDef
> 2) curl --user admin:admin http://127.0.0.1:21000/api/atlas/v2/types/typedefs/ retuens
the relationshipdef
> 3) curl --user admin:admin http://127.0.0.1:21000/api/atlas/v2/types/relationshipdef/name/Testrel4
returns the relationshipdef
> 
> 
> Sample json
> "{
>     "enumDefs": [],
>     "structDefs": [],
>     "classificationDefs": [],
>     "entityDefs": [
>       {
>               "name": "TestEnt11",
>               "superTypes": [],
>               "typeVersion": "1.0",
>               "attributeDefs": [
>                   {
>                       "name": "name",
>                       "typeName": "string",
>                       "cardinality": "SINGLE",
>                       "isIndexable": true,
>                       "isOptional": false,
>                       "isUnique": false
>                   }
>                 ]
>     },
>     {
>         "name": "TestEnt12",
>         "superTypes": [],
>         "typeVersion": "1.0",
>         "attributeDefs": [
>             {
>                 "name": "name",
>                 "typeName": "string",
>                 "cardinality": "SINGLE",
>                 "isIndexable": true,
>                 "isOptional": false,
>                 "isUnique": false
>             }
>           ]
> 
>     }
>     ],
>     "relationshipDefs" :[
>         {
>             "name": "Testrel4",
>             "typeVersion": "1.0",
> 
>             "endPointDef1": {
>               "name": "aaa",
>               "type": "TestEnt11",
>               "cardinality":"SINGLE"
>             },
>             "endPointDef2": {
>               "name": "bbb",
>               "type": "TestEnt12",
>               "cardinality":"SINGLE"
>             },
>             "relationshipCategory":"ASSOCIATION",
>             "attributeDefs": [
>                 {
>                     "name": "testattr1",
>                     "typeName": "string",
>                     "cardinality": "SINGLE",
>                     "isIndexable": true,
>                     "isOptional": true,
>                     "isUnique": false
>                 }
>               ]
>         }
>     ]
> }
> "
> 
> 
> Thanks,
> 
> David Radley
> 
>


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