You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Suma Shivaprasad <su...@gmail.com> on 2017/02/27 20:41:36 UTC

Review Request 57103: Implementation for classification in V2 API

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

Review request for atlas.


Bugs: ATLAS-1601
    https://issues.apache.org/jira/browse/ATLAS-1601


Repository: atlas


Description
-------

Add support for classification store, REST API for addition, deletion and gets


Diffs
-----

  intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
  repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
  webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 

Diff: https://reviews.apache.org/r/57103/diff/


Testing
-------

Tested through TestEntityREST


Thanks,

Suma Shivaprasad

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/#review166962
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java (line 52)
<https://reviews.apache.org/r/57103/#comment239061>

    Classes referenced by newly added import don't seem to be used. Please review and update.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java (line 128)
<https://reviews.apache.org/r/57103/#comment239066>

    consider sending 'context' as parameter to addClassification(), so that mapClassification() can use this context (and not create a new context).



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java (line 192)
<https://reviews.apache.org/r/57103/#comment239062>

    "entityType" ==> "classificationType"



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java (line 798)
<https://reviews.apache.org/r/57103/#comment239064>

    Consider moving line #798 to outside this for() loop - @#792



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java (line 214)
<https://reviews.apache.org/r/57103/#comment239071>

    "new AtlasEntitiesWithExtInfo()" - consider sending null, as this is not a mandatory parameter in lower level methods that map attribute values.


- Madhan Neethiraj


On Feb. 27, 2017, 8:41 p.m., Suma Shivaprasad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57103/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2017, 8:41 p.m.)
> 
> 
> Review request for atlas.
> 
> 
> Bugs: ATLAS-1601
>     https://issues.apache.org/jira/browse/ATLAS-1601
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Add support for classification store, REST API for addition, deletion and gets
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
>   webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 
> 
> Diff: https://reviews.apache.org/r/57103/diff/
> 
> 
> Testing
> -------
> 
> Tested through TestEntityREST
> 
> 
> Thanks,
> 
> Suma Shivaprasad
> 
>

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/#review166989
-----------------------------------------------------------


Ship it!




Ship It!

- Madhan Neethiraj


On Feb. 28, 2017, 1:58 a.m., Suma Shivaprasad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57103/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 1:58 a.m.)
> 
> 
> Review request for atlas.
> 
> 
> Bugs: ATLAS-1601
>     https://issues.apache.org/jira/browse/ATLAS-1601
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Add support for classification store, REST API for addition, deletion and gets
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
>   repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 621b32f 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
>   webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java d7adb3a 
>   webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 
> 
> Diff: https://reviews.apache.org/r/57103/diff/
> 
> 
> Testing
> -------
> 
> Tested through TestEntityREST
> 
> 
> Thanks,
> 
> Suma Shivaprasad
> 
>

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/#review167023
-----------------------------------------------------------


Ship it!




Ship It!

- Madhan Neethiraj


On Feb. 28, 2017, 7:01 a.m., Suma Shivaprasad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57103/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 7:01 a.m.)
> 
> 
> Review request for atlas.
> 
> 
> Bugs: ATLAS-1601
>     https://issues.apache.org/jira/browse/ATLAS-1601
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Add support for classification store, REST API for addition, deletion and gets
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
>   repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 621b32f 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
>   webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java d7adb3a 
>   webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 
> 
> Diff: https://reviews.apache.org/r/57103/diff/
> 
> 
> Testing
> -------
> 
> Tested through TestEntityREST
> 
> 
> Thanks,
> 
> Suma Shivaprasad
> 
>

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Vimal Sharma <vi...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/#review167035
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java (line 847)
<https://reviews.apache.org/r/57103/#comment239172>

    Can result of traitNames be put into a HashMap instead so that we can avoid the expensive membership check operation in validateClassificationExists


- Vimal Sharma


On Feb. 28, 2017, 7:01 a.m., Suma Shivaprasad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57103/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 7:01 a.m.)
> 
> 
> Review request for atlas.
> 
> 
> Bugs: ATLAS-1601
>     https://issues.apache.org/jira/browse/ATLAS-1601
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Add support for classification store, REST API for addition, deletion and gets
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
>   repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 621b32f 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
>   webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java d7adb3a 
>   webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 
> 
> Diff: https://reviews.apache.org/r/57103/diff/
> 
> 
> Testing
> -------
> 
> Tested through TestEntityREST
> 
> 
> Thanks,
> 
> Suma Shivaprasad
> 
>

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Suma Shivaprasad <su...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/
-----------------------------------------------------------

(Updated Feb. 28, 2017, 9:05 p.m.)


Review request for atlas.


Changes
-------

Fixed RC


Bugs: ATLAS-1601
    https://issues.apache.org/jira/browse/ATLAS-1601


Repository: atlas


Description
-------

Add support for classification store, REST API for addition, deletion and gets


Diffs (updated)
-----

  intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
  repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 621b32f 
  repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
  webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java d7adb3a 
  webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 

Diff: https://reviews.apache.org/r/57103/diff/


Testing
-------

Tested through TestEntityREST


Thanks,

Suma Shivaprasad

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Suma Shivaprasad <su...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/
-----------------------------------------------------------

(Updated Feb. 28, 2017, 7:01 a.m.)


Review request for atlas.


Changes
-------

fixed review comments


Bugs: ATLAS-1601
    https://issues.apache.org/jira/browse/ATLAS-1601


Repository: atlas


Description
-------

Add support for classification store, REST API for addition, deletion and gets


Diffs (updated)
-----

  intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
  repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 621b32f 
  repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
  webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java d7adb3a 
  webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 

Diff: https://reviews.apache.org/r/57103/diff/


Testing
-------

Tested through TestEntityREST


Thanks,

Suma Shivaprasad

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Apoorv Naik <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/#review166987
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java (line 466)
<https://reviews.apache.org/r/57103/#comment239082>

    Arrays.asList or Collection.singletonList would be better in terms of readability



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java (line 197)
<https://reviews.apache.org/r/57103/#comment239083>

    Can we use AtlasGraphUtilsV1.setProperty(ret, Constants.SUPER_TYPES_PROPERTY_KEY, entityType.getAllSuperTypes()); (if it does the same thing)?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java (line 801)
<https://reviews.apache.org/r/57103/#comment239084>

    traitInstanceVertex -> classificationInstanceVertex



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java (line 848)
<https://reviews.apache.org/r/57103/#comment239085>

    Would it be better to perform the exists check before processing so that we can avoid graph operations and rollbacks ?



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java (line 224)
<https://reviews.apache.org/r/57103/#comment239086>

    I think this might be misuse of Optional, usually only return types need to be optional so that the return values can be evaluated quickly and bail out if needed.



repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java (line 276)
<https://reviews.apache.org/r/57103/#comment239087>

    Same here (for Optional as parameter)



webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java (line 209)
<https://reviews.apache.org/r/57103/#comment239088>

    JavaDoc says classifications but I think this will return a specific one. Please check.



webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java (line 238)
<https://reviews.apache.org/r/57103/#comment239089>

    Maybe a better name than clss



webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java (line 270)
<https://reviews.apache.org/r/57103/#comment239090>

    Shouldn't be consuming anything here, usually DELETEs don't accept any body.



webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java (line 348)
<https://reviews.apache.org/r/57103/#comment239091>

    "empty entity list" -> "empty guid list"


- Apoorv Naik


On Feb. 28, 2017, 1:58 a.m., Suma Shivaprasad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57103/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 1:58 a.m.)
> 
> 
> Review request for atlas.
> 
> 
> Bugs: ATLAS-1601
>     https://issues.apache.org/jira/browse/ATLAS-1601
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> Add support for classification store, REST API for addition, deletion and gets
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
>   repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 621b32f 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
>   repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
>   webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java d7adb3a 
>   webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 
> 
> Diff: https://reviews.apache.org/r/57103/diff/
> 
> 
> Testing
> -------
> 
> Tested through TestEntityREST
> 
> 
> Thanks,
> 
> Suma Shivaprasad
> 
>

Re: Review Request 57103: Implementation for classification in V2 API

Posted by Suma Shivaprasad <su...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57103/
-----------------------------------------------------------

(Updated Feb. 28, 2017, 1:58 a.m.)


Review request for atlas.


Changes
-------

Fixed review comments


Bugs: ATLAS-1601
    https://issues.apache.org/jira/browse/ATLAS-1601


Repository: atlas


Description
-------

Add support for classification store, REST API for addition, deletion and gets


Diffs (updated)
-----

  intg/src/main/java/org/apache/atlas/AtlasErrorCode.java d58c514 
  repository/src/main/java/org/apache/atlas/repository/converters/AtlasInstanceConverter.java 621b32f 
  repository/src/main/java/org/apache/atlas/repository/store/graph/AtlasEntityStore.java 6c372b3 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStoreV1.java c84f169 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java 09f69db 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java 3ba2190 
  repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityMutationContext.java 23e825e 
  webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java d7adb3a 
  webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java d1bef78 

Diff: https://reviews.apache.org/r/57103/diff/


Testing
-------

Tested through TestEntityREST


Thanks,

Suma Shivaprasad