Re: Review Request 66542: ATLAS-2534: Atlas glossary

2018-04-15 Thread Madhan Neethiraj 

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


Ship it!




Ship It!

- Madhan Neethiraj


On April 11, 2018, 6:18 a.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66542/
> ---
> 
> (Updated April 11, 2018, 6:18 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, keval bhatt, Madhan Neethiraj, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2534
> https://issues.apache.org/jira/browse/ATLAS-2534
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> This is an enhancement to the existing glossary code.
> Some refactoring has been done to move helper methods into GlossaryUtils, 
> GlossaryTermUtils and GlossaryCategoryUtils classes for easier maintenance.
> term assignment APIs deal with AtlasRelatedObjectId instead of 
> AtlasEntityHeader
> Delete functionality has been improved
> 
> 
> Diffs
> -
> 
>   common/src/main/java/org/apache/atlas/repository/Constants.java 7ccc748b5 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java c6be3622d 
>   intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossaryTerm.java 
> f42bf3552 
>   
> intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasRelatedCategoryHeader.java
>  aa7a6b191 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java
>  36fb0bcc2 
>   
> repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryService.java 
> 49c8f51a6 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryTermUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/ogm/DataAccess.java 
> 14737499a 
>   
> repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryTermDTO.java
>  3f8bfe289 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
>  7b2806e8d 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java
>  9add9b52e 
>   repository/src/test/java/org/apache/atlas/glossary/GlossaryServiceTest.java 
> 62a0cb461 
>   webapp/src/main/java/org/apache/atlas/web/rest/GlossaryREST.java 6779dee31 
> 
> 
> Diff: https://reviews.apache.org/r/66542/diff/6/
> 
> 
> Testing
> ---
> 
> mvn clean package executes successfully (except cassandra failure, I've 
> disable that on my system)
> 
> PreCommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/262/
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 66542: ATLAS-2534: Atlas glossary

2018-04-13 Thread Apoorv Naik 


> On April 12, 2018, 11:19 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
> > Lines 84 (patched)
> > 
> >
> > if only relationship attributes changed (i.e. anchor entity remains 
> > same), then there is no need to delete/recreate relationship. Updating 
> > existing instance should do.

Anchor relation doesn't have any attributes hence the only possible update can 
be a change of anchor


> On April 12, 2018, 11:19 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
> > Lines 133 (patched)
> > 
> >
> > If a category is relocated inside another category, existing 
> > relationshipEdge should be removed and create a new edge between the 
> > category and its new parent. Please review.

Good catch, fixed.


- Apoorv


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


On April 11, 2018, 6:18 a.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66542/
> ---
> 
> (Updated April 11, 2018, 6:18 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, keval bhatt, Madhan Neethiraj, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2534
> https://issues.apache.org/jira/browse/ATLAS-2534
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> This is an enhancement to the existing glossary code.
> Some refactoring has been done to move helper methods into GlossaryUtils, 
> GlossaryTermUtils and GlossaryCategoryUtils classes for easier maintenance.
> term assignment APIs deal with AtlasRelatedObjectId instead of 
> AtlasEntityHeader
> Delete functionality has been improved
> 
> 
> Diffs
> -
> 
>   common/src/main/java/org/apache/atlas/repository/Constants.java 7ccc748b5 
>   intg/src/main/java/org/apache/atlas/AtlasErrorCode.java c6be3622d 
>   intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossaryTerm.java 
> f42bf3552 
>   
> intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasRelatedCategoryHeader.java
>  aa7a6b191 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntityDiscoveryService.java
>  36fb0bcc2 
>   
> repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryService.java 
> 49c8f51a6 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryTermUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/repository/ogm/DataAccess.java 
> 14737499a 
>   
> repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryTermDTO.java
>  3f8bfe289 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java
>  7b2806e8d 
>   
> repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java
>  9add9b52e 
>   repository/src/test/java/org/apache/atlas/glossary/GlossaryServiceTest.java 
> 62a0cb461 
>   webapp/src/main/java/org/apache/atlas/web/rest/GlossaryREST.java 6779dee31 
> 
> 
> Diff: https://reviews.apache.org/r/66542/diff/5/
> 
> 
> Testing
> ---
> 
> mvn clean package executes successfully (except cassandra failure, I've 
> disable that on my system)
> 
> PreCommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/262/
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 66542: ATLAS-2534: Atlas glossary

2018-04-12 Thread Madhan Neethiraj 

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




repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 43 (patched)


'final' marker on each parameter seems unnecessary. Unless it is required, 
consider removing it - for easier readability (i.e. reduced distraction).



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 47 (patched)


newObj ==> category



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 47 (patched)


newObj ==> category
 please review and update other such usage



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 63 (patched)


Instead of using generic exception code AtlasErrorCode.BAD_REQUEST (and a 
hardcoded message here), consider adding specific error codes and messages for 
each case.



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 84 (patched)


if only relationship attributes changed (i.e. anchor entity remains same), 
then there is no need to delete/recreate relationship. Updating existing 
instance should do.



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 133 (patched)


If a category is relocated inside another category, existing 
relationshipEdge should be removed and create a new edge between the category 
and its new parent. Please review.



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 203 (patched)


toCreate ==> relatedTerms



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 225 (patched)


toUpdate ==> terms



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 238 (patched)


existingTerms ==> terms



repository/src/main/java/org/apache/atlas/glossary/GlossaryTermUtils.java
Lines 45 (patched)


Expressions "DEBUG_ENABLED" and "LOG.isDebugEnabled()" are used here. It 
will help readability by sticking to one approach.



repository/src/main/java/org/apache/atlas/glossary/GlossaryUtils.java
Lines 59 (patched)


Consider replacing strings with constants:
 - "expression" ==> TERM_RELATION_ATTRIBUTE_EXPRESSION
 - "description" ==> TERM_RELATION_ATTRIBUTE_DESCRIPTION
 - "steward" ==> TERM_RELATION_ATTRIBUTE_STEWARD
 - "source" ==> TERM_RELATION_ATTRIBUTE_SOURCE
 - "status" ==> TERM_RELATION_ATTRIBUTE_STATUS



repository/src/main/java/org/apache/atlas/repository/ogm/DataAccess.java
Lines 121 (patched)


Instead of including 'e.getMessage()' in the message, consider 'e' - which 
will print the message and the stack as well. This will be helpful in 
troubleshooting.

  LOG.warn("Bulk load encountered an error", e);


- Madhan Neethiraj


On April 11, 2018, 6:18 a.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66542/
> ---
> 
> (Updated April 11, 2018, 6:18 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, keval bhatt, Madhan Neethiraj, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-2534
> https://issues.apache.org/jira/browse/ATLAS-2534
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> This is an enhancement to the existing glossary code.
> Some refactoring has been done to move helper methods into GlossaryUtils, 
> GlossaryTermUtils and GlossaryCategoryUtils classes for easier maintenance.
> term assignment APIs deal with AtlasRelatedObjectId instead of 
> AtlasEntityHeader
> Delete functionality has been improved
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossaryTerm.java 
> f42bf3552 
>   
> intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasRelatedCategoryHeader.java
>  aa7a6b191 
>   
> repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java 
> PRE-CREATION 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryService.java 
> 49c8f51a6 
>   repository/src/main/java/org/apache/atlas/glossary/GlossaryT