-----------------------------------------------------------
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)
<https://reviews.apache.org/r/66542/#comment281987>

    '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)
<https://reviews.apache.org/r/66542/#comment282006>

    newObj ==> category



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 47 (patched)
<https://reviews.apache.org/r/66542/#comment282008>

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



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 63 (patched)
<https://reviews.apache.org/r/66542/#comment281988>

    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)
<https://reviews.apache.org/r/66542/#comment282007>

    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)
<https://reviews.apache.org/r/66542/#comment281989>

    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)
<https://reviews.apache.org/r/66542/#comment282009>

    toCreate ==> relatedTerms



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 225 (patched)
<https://reviews.apache.org/r/66542/#comment282011>

    toUpdate ==> terms



repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
Lines 238 (patched)
<https://reviews.apache.org/r/66542/#comment282012>

    existingTerms ==> terms



repository/src/main/java/org/apache/atlas/glossary/GlossaryTermUtils.java
Lines 45 (patched)
<https://reviews.apache.org/r/66542/#comment281986>

    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)
<https://reviews.apache.org/r/66542/#comment281985>

    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)
<https://reviews.apache.org/r/66542/#comment281984>

    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/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/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/4/
> 
> 
> 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/258/
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Reply via email to