> On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossary.java > > Lines 144 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993066#file1993066line144> > > > > why create a new HashSet, instead of adding to exiting one? > > > > Set<AtlasRelatedTermId> terms = this.terms; > > > > if (terms == null) { > > terms = new HashSet<>(); > > > > this.terms = terms; > > } > > > > terms.add(relatedTermId); > > > > Same for categories and classifications.
Will update the code > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossaryTerm.java > > Lines 312 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993069#file1993069line312> > > > > 'examples' attribute is not handled by setAttribute()? setAttribute is only called from the REST layer, and is used for partial updates of Primitive attributes only. > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasRelatedCategoryId.java > > Lines 36 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993073#file1993073line36> > > > > is this the guid for: relationship or category? Consider renaming to > > categoryGuid or relationshipGuid. Sure > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasRelatedTermId.java > > Lines 39 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993074#file1993074line39> > > > > is this the guid for: relationship or term? Consider renaming to > > termGuid or relationshipGuid. Sure. > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java > > Lines 87 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993075#file1993075line87> > > > > Instead of using AtlasGlossaryTerm (which has references to all > > entities the term is assigned to, all terms the term is related to), > > consider using AtlasRelatedTermId (or an equivalent). Will update. > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryDTO.java > > Lines 120 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993086#file1993086line120> > > > > not handling relationship attributes here? from() method above handles > > relationship attributes. Setting relationship attributes causes the store to error out when handling them, hence they're only handled during the conversion from entity to POJO. > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryDTO.java > > Lines 135 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993086#file1993086line135> > > > > Shouldn't extInfo not include all entities referenced by relationship > > attributes? Same as above > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryTermDTO.java > > Lines 97 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993087#file1993087line97> > > > > Consider adding toRelatedTermIdsSet() method and replace lines #96-#250 > > with: > > > > ret.setSeeAlso(toRelatedTermIdsSet(entity.getRelationshipAttribute("seeAlso"))); > > > > ret.setSynonyms(toRelatedTermIdsSet(entity.getRelationshipAttribute("synonyms"))); > > > > ret.setAntonyms(toRelatedTermIdsSet(entity.getRelationshipAttribute("antonyms"))); > > > > ret.setPreferredTerms(toRelatedTermIdsSet(entity.getRelationshipAttribute("preferredTerms"))); > > > > ret.setPreferredToTerms(toRelatedTermIdsSet(entity.getRelationshipAttribute("preferredToTerms"))); > > > > ret.setReplacementTerms(toRelatedTermIdsSet(entity.getRelationshipAttribute("replacementTerms"))); > > > > ret.setReplacedBy(toRelatedTermIdsSet(entity.getRelationshipAttribute("replacedBy"))); > > > > ret.setTranslationTerms(toRelatedTermIdsSet(entity.getRelationshipAttribute("translationTerms"))); > > > > ret.setTranslatedTerms(toRelatedTermIdsSet(entity.getRelationshipAttribute("translatedTerms"))); > > > > ret.setIsA(toRelatedTermIdsSet(entity.getRelationshipAttribute("isA"))); > > > > ret.setClassify(toRelatedTermIdsSet(entity.getRelationshipAttribute("classify"))); > > > > ret.setValidValues(toRelatedTermIdsSet(entity.getRelationshipAttribute("validValues"))); > > > > ret.setValidValuesFor(toRelatedTermIdsSet(entity.getRelationshipAttribute("validValuesFor"))); > > > > > > > > > > > > Set<AtlasRelatedTermId> toRelatedTermIdsSet(Object obj) { > > Set<AtlasRelatedTermId> ret = null; > > > > if (obj instanceof Collection) { > > ret = new HashSet<>(); > > > > for (Object elem : (Collection) obj) { > > if (elem instanceof AtlasRelatedObjectId) { > > ret.add(toRelatedTermId((AtlasRelatedObjectId) elem); > > } > > } > > } > > > > return ret; > > } Will update the implementation > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryTermDTO.java > > Lines 286 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993087#file1993087line286> > > > > not handling relationship attributes here? Setting relationship attributes causes the store to error out when handling them, hence they're only handled during the conversion from entity to POJO. > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryTermDTO.java > > Lines 302 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993087#file1993087line302> > > > > Shouldn't extInfo not include all entities referenced by relationship > > attributes? Same as above > On April 5, 2018, 9:44 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java > > Lines 71 (patched) > > <https://reviews.apache.org/r/66478/diff/1/?file=1993089#file1993089line71> > > > > Consider using Constants.QUALIFIED_NAME, instead of introducing a new > > const. Will do. - Apoorv ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66478/#review200563 ----------------------------------------------------------- On April 5, 2018, 8:10 p.m., Apoorv Naik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66478/ > ----------------------------------------------------------- > > (Updated April 5, 2018, 8:10 p.m.) > > > Review request for atlas, keval bhatt, Madhan Neethiraj, and Sarath > Subramanian. > > > Bugs: ATLAS-2534 > https://issues.apache.org/jira/browse/ATLAS-2534 > > > Repository: atlas > > > Description > ------- > > Glossary implementation, no UI > > > Diffs > ----- > > addons/models/0000-Area0/0010-base_model.json aebe955db > addons/models/0300-Area3-SubjectArea/0330-Terms.json f492ddfdf > addons/models/0300-Area3-SubjectArea/0350-RelatedTerms.json d88f57c20 > > graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasGraphQuery.java > 7bdbeabf0 > > graphdb/common/src/main/java/org/apache/atlas/repository/graphdb/tinkerpop/query/NativeTinkerpopGraphQuery.java > 75665592e > > graphdb/common/src/main/java/org/apache/atlas/repository/graphdb/tinkerpop/query/TinkerpopGraphQuery.java > 96b9705fa > > graphdb/common/src/main/java/org/apache/atlas/repository/graphdb/tinkerpop/query/expr/OrderByPredicate.java > PRE-CREATION > > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraph.java > 6820a93c2 > > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/query/NativeJanusGraphQuery.java > d3c976df5 > > graphdb/titan0/src/main/java/org/apache/atlas/repository/graphdb/titan0/query/NativeTitan0GraphQuery.java > 2903ae228 > intg/src/main/java/org/apache/atlas/AtlasErrorCode.java 997ac68fb > intg/src/main/java/org/apache/atlas/model/AtlasBaseModelObject.java > 688f6f4d2 > intg/src/main/java/org/apache/atlas/model/annotation/AtlasJSON.java > PRE-CREATION > intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossary.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossaryBaseObject.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossaryCategory.java > PRE-CREATION > intg/src/main/java/org/apache/atlas/model/glossary/AtlasGlossaryTerm.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/glossary/enums/AtlasTermAssignmentStatus.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/glossary/enums/AtlasTermRelationshipStatus.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasGlossaryId.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasRelatedCategoryId.java > PRE-CREATION > > intg/src/main/java/org/apache/atlas/model/glossary/relations/AtlasRelatedTermId.java > PRE-CREATION > intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java > 395431922 > intg/src/main/java/org/apache/atlas/model/instance/AtlasRelationship.java > 576847f6e > intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java > 61168f63f > repository/src/main/java/org/apache/atlas/glossary/GlossaryService.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/ogm/AbstractDataTransferObject.java > f1a8bc91d > > repository/src/main/java/org/apache/atlas/repository/ogm/AtlasSavedSearchDTO.java > a1a8f598d > > repository/src/main/java/org/apache/atlas/repository/ogm/AtlasUserProfileDTO.java > bcf2b9d27 > repository/src/main/java/org/apache/atlas/repository/ogm/DTORegistry.java > 818960d96 > repository/src/main/java/org/apache/atlas/repository/ogm/DataAccess.java > c99d2f836 > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AbstractGlossaryDTO.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryCategoryDTO.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryDTO.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/ogm/glossary/AtlasGlossaryTermDTO.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityChangeNotifier.java > b9945d4fc > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java > 13ee2a6df > > repository/src/main/java/org/apache/atlas/repository/userprofile/UserProfileService.java > a428b92ee > repository/src/test/java/org/apache/atlas/TestModules.java c901e891c > repository/src/test/java/org/apache/atlas/glossary/GlossaryServiceTest.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/glossary/PaginationHelperTest.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/repository/impexp/ZipFileResourceTestUtils.java > d8e916d77 > > webapp/src/main/java/org/apache/atlas/notification/EntityNotificationListenerV2.java > 01a95cf80 > webapp/src/main/java/org/apache/atlas/web/rest/GlossaryREST.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/66478/diff/2/ > > > Testing > ------- > > Added test GlossaryServiceTest, runs successfully. > > mvn clean package executes successfully. > > PreCommit: > https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/235/ > (in-progress) > > > Thanks, > > Apoorv Naik > >
