atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Radley <david_rad...@uk.ibm.com>
Subject Re: Review Request 59719: ATLAS-1852 Create RelationshipDef
Date Sun, 11 Jun 2017 11:13:49 GMT


> On June 9, 2017, 4:06 p.m., Madhan Neethiraj wrote:
> > intg/src/main/java/org/apache/atlas/model/typedef/AtlasRelationshipDef.java
> > Lines 182 (patched)
> > <https://reviews.apache.org/r/59719/diff/2/?file=1746756#file1746756line182>
> >
> >     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.

makes sense - I have moved the validations


> On June 9, 2017, 4:06 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java
> > Lines 273 (patched)
> > <https://reviews.apache.org/r/59719/diff/2/?file=1746770#file1746770line279>
> >
> >     '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).

removed all of these files  - apart for the typeCategory RELATIONSHIP enum.


> On June 9, 2017, 4:06 p.m., Madhan Neethiraj wrote:
> > webapp/src/main/java/org/apache/atlas/web/rest/TypesREST.java
> > Line 22 (original), 22 (patched)
> > <https://reviews.apache.org/r/59719/diff/2/?file=1746780#file1746780line22>
> >
> >     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.

reverting this line gives import errors. So I think is needs more than the original imports.


- David


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


On June 10, 2017, 10:22 p.m., David Radley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59719/
> -----------------------------------------------------------
> 
> (Updated June 10, 2017, 10: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.

> This functions works
> 1) create relationshipDef
> 2) get typedefs
> 3) get typedef headers
> 4) get relationshgipdef by name
> 5) get relationshipDef by guid.
> 6) delete relationshipDef
> 
> This is yet to do:
> 1) create after a delete 
> 2) updates do not work
> 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

>   intg/src/test/java/org/apache/atlas/type/TestAtlasRelationshipType.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/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 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/DataTypes.java 21d5f1a1e7488c73ab84ec9512d488ed3b9002bf

>   webapp/src/main/java/org/apache/atlas/web/resources/TypesResource.java 08121d8d9c0ed34f62a9e4d49c4be87a98639907

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

> 
> 
> Diff: https://reviews.apache.org/r/59719/diff/3/
> 
> 
> Testing
> -------
> 
> Junits complete successfully
> 1) create relationshipDef
> 2) get typedefs
> 3) get typedef headers
> 4) get relationshgipdef by name
> 5) get relationshipDef by guid.
> 6) delete relationshipDef
> 
> Delete is successful in as far as the get typedefs does not show the relationshipDef.
But a subsequent create fails as it thinks the vertex exists. Investigating.
> 
> 
> Thanks,
> 
> David Radley
> 
>


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