You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Apoorv Naik <na...@gmail.com> on 2018/04/11 06:18:16 UTC
Review Request 66542: ATLAS-2534: Atlas glossary
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66542/
-----------------------------------------------------------
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
repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java PRE-CREATION
repository/src/main/java/org/apache/atlas/glossary/GlossaryService.java 91abf5e71
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 e00c3e953
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/1/
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/247/
Thanks,
Apoorv Naik
Re: Review Request 66542: ATLAS-2534: Atlas glossary
Posted by Apoorv Naik <na...@gmail.com>.
> On April 12, 2018, 11:19 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/glossary/GlossaryCategoryUtils.java
> > Lines 84 (patched)
> > <https://reviews.apache.org/r/66542/diff/3/?file=1996814#file1996814line84>
> >
> > 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)
> > <https://reviews.apache.org/r/66542/diff/3/?file=1996814#file1996814line133>
> >
> > 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
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 66542: ATLAS-2534: Atlas glossary
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
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
>
>