----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70789/#review215718 -----------------------------------------------------------
Thanks for doing this so quickly. I do have some suggestions. thanks, Sridhar graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphSolrIndexClient.java Line 38 (original), 38 (patched) <https://reviews.apache.org/r/70789/#comment302521> I liked original name. If any one in community wants implement other AtlasGZraphIndexClients, they can add their own. graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphSolrIndexClient.java Lines 52 (patched) <https://reviews.apache.org/r/70789/#comment302524> I like it. graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphSolrIndexClient.java Line 63 (original), 61 (patched) <https://reviews.apache.org/r/70789/#comment302523> Thanks for noticing this. repository/src/main/java/org/apache/atlas/discovery/FreeTextSearchProcessor.java Line 53 (original) <https://reviews.apache.org/r/70789/#comment302522> I am not sure why you removed it. Ofcourse, it is an optimization. repository/src/main/java/org/apache/atlas/discovery/FreeTextSearchProcessor.java Line 95 (original), 70 (patched) <https://reviews.apache.org/r/70789/#comment302525> thx. repository/src/main/java/org/apache/atlas/discovery/SearchContext.java Line 79 (original), 77 (patched) <https://reviews.apache.org/r/70789/#comment302526> Never felt good with static block. Thanks for finding a better place for this. repository/src/main/java/org/apache/atlas/discovery/SearchContext.java Lines 124 (patched) <https://reviews.apache.org/r/70789/#comment302527> I prefer to not have so many lines. If funcional separation is needed, can we remove one line here? repository/src/main/java/org/apache/atlas/repository/graph/SolrIndexHelper.java Line 109 (original), 113 (patched) <https://reviews.apache.org/r/70789/#comment302528> I know what you wanted to do here. But, this is user entered value, I would prefer it to be throwing an exception early on so that the developers can notice it and fix it. Otherwise, they might not realize their mistake if eat up the issue silently without throwing exception. repository/src/main/java/org/apache/atlas/repository/patches/FreeTextRequestHandlerPatch.java Lines 27 (patched) <https://reviews.apache.org/r/70789/#comment302530> Can we remove this class initialize the SolrIndexHelper in PatchManager instead. I am not sure, if we want this to be created in a patch setting. I rather have it done explicitly AtlasPatchManager after going through all patches in the system. Otherwise, the new patches might be added after this patch and we might some def types or patches--may be listerner in GraphBackedIndexHelper will cover this scenario. But, I still think that this should be initialized by PatchManager after all patches are run as an explicit step. repository/src/main/java/org/apache/atlas/repository/patches/FreeTextRequestHandlerPatch.java Lines 31 (patched) <https://reviews.apache.org/r/70789/#comment302529> searche ==> searches. - Sridhar K On June 6, 2019, 7:41 a.m., Madhan Neethiraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70789/ > ----------------------------------------------------------- > > (Updated June 6, 2019, 7:41 a.m.) > > > Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, keval bhatt, > Sridhar K, Le Ma, Nikhil Bonte, Nixon Rodrigues, Saqeeb Shaikh, and Sarath > Subramanian. > > > Bugs: ATLAS-3262 > https://issues.apache.org/jira/browse/ATLAS-3262 > > > Repository: atlas > > > Description > ------- > > - skipped full-text index computation & population when free-text is enabled > - added FreeTextRequestHandlerPatch to create /freetext request handler in > Solr > - fixed free-text search handling of typeName filter > > > Diffs > ----- > > > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraph.java > edab08cc0 > > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphSolrIndexClient.java > a55fc3689 > intg/src/main/java/org/apache/atlas/ApplicationProperties.java 6d0153f7d > > repository/src/main/java/org/apache/atlas/discovery/FreeTextSearchProcessor.java > b1a84c742 > repository/src/main/java/org/apache/atlas/discovery/SearchContext.java > 50edb9faf > > repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java > 56655a845 > > repository/src/main/java/org/apache/atlas/repository/graph/SolrIndexHelper.java > 20a517ff1 > > repository/src/main/java/org/apache/atlas/repository/patches/AtlasPatchManager.java > 259f24674 > > repository/src/main/java/org/apache/atlas/repository/patches/FreeTextRequestHandlerPatch.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityChangeNotifier.java > 035b02ce4 > > repository/src/main/java/org/apache/atlas/util/AtlasRepositoryConfiguration.java > 1d296057b > > > Diff: https://reviews.apache.org/r/70789/diff/1/ > > > Testing > ------- > > - verified that free-text handler is created by the patch for the first time > this enhancement is applied in an existing env > - verified that the free-text search handles typeName filter correctly > - pre-commit tests run: > https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1176/ > > > Thanks, > > Madhan Neethiraj > >
