> On July 24, 2020, 7:14 a.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java > > Lines 67 (patched) > > <https://reviews.apache.org/r/72666/diff/8/?file=2236219#file2236219line67> > > > > incrementCapacityBy is already an instance member, line #29 above. It > > is unnecessary to pass it as parameter to methods at #67, #79. Please > > review and update.
I removed the initial capacity from the class altogether. > On July 24, 2020, 7:14 a.m., Madhan Neethiraj wrote: > > intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java > > Lines 73 (patched) > > <https://reviews.apache.org/r/72666/diff/8/?file=2236219#file2236219line73> > > > > This assumes that access is done in strict ascending order sequence, > > without any gaps. This is not a good assumption for a generic/reusable > > class implementation. This will be broken with the following access pattern: > > FixedBufferList<MyObjecy> list = new FixedBufferList<>(10, 1); > > > > list.getForUpdate(0); > > list.getForUpdate(15); > > > > Please review my earlier comment and update to remove this assumption. > > Also, I suggest to collapse following methods into one, named > > ensureCapacity(): > > - request() > > - ensureCapacity(): it not obvious what the return value of this > > method is, and it is critical for the caller at #74. All this noise, and > > confusion can be avoided by simply collapsing this method into previous > > method at #67. > > - instantiateItems(): this method is not callable from any other > > context, as it simply "adds()" to the buffer I re-worked the interface. There is no way to pass index. > On July 24, 2020, 7:14 a.m., Madhan Neethiraj wrote: > > webapp/src/main/java/org/apache/atlas/notification/EntityNotificationListenerV2.java > > Line 168 (original), 175 (patched) > > <https://reviews.apache.org/r/72666/diff/8/?file=2236222#file2236222line175> > > > > sendNotifications() can be called from the same thread multiple times - > > for example when a transaction involves create/update/delete of entities > > i.e. a graph transaction can call more than one of the following methods: > > - onEntitiesAdded() > > - onEntitiesUpdated() > > - onEntitiesDeleted() > > - onEntitiesPurged() > > - onClassificationsAdded() > > - onClassificationsUpdated() > > - onClassificationsDeleted() > > > > Each call would end up calling EntityNotificationSender.send() with the > > notification objects, which in turn stores the objects in a list. The > > second call in the same transaction will end up overwriting > > EntityNotificationV2 instances in list buffer - which will cause the > > notification objects stored in EntityNotificationSender.send() be updated > > as well. This will result in incorrect notification to be sent. Please > > review this carefully and update. I addressed by eliminating usage of FixedBufferList from this class. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72666/#review221351 ----------------------------------------------------------- On July 24, 2020, 5:18 a.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72666/ > ----------------------------------------------------------- > > (Updated July 24, 2020, 5:18 a.m.) > > > Review request for atlas, Madhan Neethiraj, Nikhil Bonte, Nixon Rodrigues, > and Sarath Subramanian. > > > Bugs: ATLAS-3878 > https://issues.apache.org/jira/browse/ATLAS-3878 > > > Repository: atlas > > > Description > ------- > > **Background** > See JIRA for details. > > *Analysis* Using memory profiling tools, it was observed that large number of > notification objects were created. These stayed in memory and later were > promoted to higher generation, thereby taking even longer to be collected. > > **Approach** > Using the fixed-buffer approach to address the problem of creating large > number of small objects. > > New *FixedBufferList* This is an encapsulation over *ArrayList*. During > initial allocation, list is populated with default values. Features: > - Setting of values to these pre-allocated objects is achieved by first doing > a *get* on the element and then assigning values to it. > - *toList* fetches the sub-list from the encapsulating list. This uses the > state within the class to fetch the right length for the returning array. > > New *NamedFixedBufferList* Maintains a per-thread *FixedBufferList*. This is > necessary since the list is now part class's state. > Modified *EntityAuditListenerV2* Uses the new classes. > Modifed *EntityNotificationListener* Uses the new classes. > > **Verification** > - Using the test setup, the memory usage was observed over a period of 24 > hrs. > - Memory usage and object allocation was obvserved using memory profiler. > > > Diffs > ----- > > intg/src/main/java/org/apache/atlas/AtlasConfiguration.java 2c007ca01 > intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java PRE-CREATION > intg/src/test/java/org/apache/atlas/utils/FixedBufferListTest.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditListenerV2.java > 79527acfa > > webapp/src/main/java/org/apache/atlas/notification/EntityNotificationListenerV2.java > a677b315c > > > Diff: https://reviews.apache.org/r/72666/diff/8/ > > > Testing > ------- > > **Unit testing** > Unit tests added for the new classes. > > **Volume testing** > Setup: > - Node: Threads 40, Core: 40, Allocated Memory: 12 GB > - Multiple Kafka queues ingesting data. > - Bulk entity creation using custom script ingesting 100M entities. > > Memory usage stayed between 0 and 5% during the 24 hr period. > > With: > - Workers: 64 > - Batch size: 50 (fewer elements in batch improve commit time and audit write > time). > - Throughput: ~1.2 M entities per hour. Without out of memory error. > > **Pre-commit** > https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/2035/ > https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/2067/ > > > Thanks, > > Ashutosh Mestry > >
