----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51092/#review162884 -----------------------------------------------------------
repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java (line 268) <https://reviews.apache.org/r/51092/#comment234267> Looks like this "for" loop can be replaced with a call to: idToVertexMap.putAll(foundVertices) Please review. repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java (line 277) <https://reviews.apache.org/r/51092/#comment234260> Given there is no 'else' for this large 'if' block, please consider using continue, as shown below. I think it will help readability of the code. if (processedIds.contains(id)) { continue; } repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java (line 283) <https://reviews.apache.org/r/51092/#comment234262> Consider surronding "LOG.debug()" with "if (LOG.isDebugEnabled())" - toShortString() implementations are not as simple/efficient as it sounds - they perform String.format(..).! repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java (line 294) <https://reviews.apache.org/r/51092/#comment234263> Consider surronding "LOG.debug()" with "if (LOG.isDebugEnabled())" - repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java (line 295) <https://reviews.apache.org/r/51092/#comment234265> Should instance be a "ReferenceableInstance"? Or can it be any inplementation of "ITypedReferenceableInstance" - like "Id", "ReferenceableInstance"? This check is inconsistent with the exception message below. Once of these needs to be corrected. repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java (line 299) <https://reviews.apache.org/r/51092/#comment234266> Since this is an existing instance, it might be good to replace variable name 'newInstance' with a local variable like 'exisingInstance' here. While at this, consider moving line 281 inside "if (instanceVertex == null)" block. repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java (line 334) <https://reviews.apache.org/r/51092/#comment234268> Please use "if (LOG.isDebugEnabled())" here, to avoid overhead of string(instanceVertex). - Madhan Neethiraj On Jan. 24, 2017, 11:22 p.m., Jeff Hagelberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51092/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2017, 11:22 p.m.) > > > Review request for atlas and David Kantor. > > > Repository: atlas > > > Description > ------- > > Apply performance fixes for create/update entities from IBM fork to Atlas. > During our performance profiling, we found a number of performance hotspots > in JProfiler. Our main findings were > > - multiple queries were being executed for each instance being > created/updated to find matches by unique attribute. > - one query was being executed for each instance being created/updated to > find the corresponding vertex if there is one > - Calculating the value of the full text property was taking a > significant portion of the time to create/update entities, mainly due to its > calls to getVertexForGUID > > The changes we put in do the following: > > - batch lookups by guid when create/update entities. Execute one > AtlasGraphQuery to find them all. > - batch lookups by unique attribute when create/update entities. Execute > one AtlasGraphQuery per class to find unique attribute matches. > - find all existing vertices up front during create/update entity. Use > those vertices during the graph mapping process to avoid running unnecessary > graph queries > - reuse reference vertices from instance to graph mapping when computing > full text property > > Also, resolved all test failures in webapp. I disentagled the three > competing versions of the hive model that the various tests were trying to > use. Now they all pass. I tried to follow the path of least resistence. We > really should clean this up more, there is really no need for threee > different versions of hive_table and its related classes. > > > Diffs > ----- > > > repository/src/main/java/org/apache/atlas/repository/audit/InMemoryEntityAuditRepository.java > 50a007bf1bf7e8c07a57b327b3aa3b907dd5f660 > > repository/src/main/java/org/apache/atlas/repository/graph/FullTextMapper.java > 911b1adbad92a76ce15f49ce023b56aeca8b4f94 > > repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedMetadataRepository.java > 0c80aeddbae35c80cdf8b83cea6aefadf6454a20 > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > 889236ca805142a93d9d4e63789fb0cc9aea05aa > > repository/src/main/java/org/apache/atlas/repository/graph/TypedInstanceToGraphMapper.java > 4e55bbcab91b8572d13345cec61e8df2f195ee4f > > repository/src/main/java/org/apache/atlas/repository/graph/VertexLookupContext.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java > 35a489f2d77578a72a2e73a37bdf094af25a166e > repository/src/main/java/org/apache/atlas/util/AttributeValueMap.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/util/IndexedInstance.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/repository/graph/GraphHelperTest.java > a7dc13db72fb4ff268312c106df1b6c41f46962f > typesystem/src/test/resources/atlas-application.properties > 108630b485e712179dc80d001dbce97551b37516 > webapp/src/main/java/org/apache/atlas/web/resources/EntityResource.java > 17c8237569746bb77f75692d50ce115c21b80c7c > > webapp/src/test/java/org/apache/atlas/notification/EntityNotificationIT.java > 1774611285956a7f187bd4353778fe34a6893c69 > > webapp/src/test/java/org/apache/atlas/notification/NotificationHookConsumerIT.java > 4a3db8874468bc8373625afe93df0a0938d00e39 > webapp/src/test/java/org/apache/atlas/web/resources/BaseResourceIT.java > 51be64c0dccecd56e720f3633ba46bbaf5c37f5d > > webapp/src/test/java/org/apache/atlas/web/resources/DataSetLineageJerseyResourceIT.java > 8334e4f9dd2eedf48e516ee34a5b2981489ce0da > > webapp/src/test/java/org/apache/atlas/web/resources/EntityDiscoveryJerseyResourceIT.java > 2bbe10a0827b2035a6c74e6b3aa3520b7e12d571 > > webapp/src/test/java/org/apache/atlas/web/resources/EntityJerseyResourceIT.java > f084053a03f9940d2f9015d4a15ef9c085553ae5 > > webapp/src/test/java/org/apache/atlas/web/resources/EntityV2JerseyResourceIT.java > 74338fd7aee2d81f54f59b0be15bd249852fbd0b > > webapp/src/test/java/org/apache/atlas/web/resources/MetadataDiscoveryJerseyResourceIT.java > b004cb52cc996763dbc4c24cd80ab545c5749358 > > Diff: https://reviews.apache.org/r/51092/diff/ > > > Testing > ------- > > Ran complete build on linux, all tests passed > > > Thanks, > > Jeff Hagelberg > >
