----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73119/#review222416 -----------------------------------------------------------
repository/src/main/java/org/apache/atlas/repository/audit/HBaseBasedAuditRepository.java Lines 235 (patched) <https://reviews.apache.org/r/73119/#comment311457> Consider replacing this duplicated method (in HBaseBasedAuditRepository and InMemoryEntityAuditRepository) with 3 Comparator implementations in EntityAuditEventV2, like: public class EntityAuditEventV2 { ... public static class UserComparator implements Comparator<EntityAuditEventV2> { @Override public int compare(EntityAuditEventV2 me, EntityAuditEventV2 other) { int ret = me.getUser().compareToIgnoreCase(other.getUser()); if (ret == 0) { ret = Long.compare(me.getTimestamp(), other.getTimestamp()); } } } public static class ActionComparator implements Comparator<EntityAuditEventV2> { @Override public int compare(EntityAuditEventV2 me, EntityAuditEventV2 other) { int ret = me.getAction().compareTo(other.getAction()); if (ret == 0) { ret = Long.compare(me.getTimestamp(), other.getTimestamp()); } } } public static class TimestampComparator implements Comparator<EntityAuditEventV2> { @Override public int compare(EntityAuditEventV2 me, EntityAuditEventV2 other) { return Long.compare(me.getTimestamp(), other.getTimestamp()); } } } And sort using: events.sort(EntityAuditEventV2.UserComparator) or events.sort(EntityAuditEventV2.ActionComparator) or events.sort(EntityAuditEventV2.TimestampComparator) or repository/src/main/java/org/apache/atlas/repository/audit/HBaseBasedAuditRepository.java Lines 300 (patched) <https://reviews.apache.org/r/73119/#comment311459> Ideally, have a single return from the method. Also, avoid single statement blocks. repository/src/main/java/org/apache/atlas/repository/audit/HBaseBasedAuditRepository.java Lines 302 (patched) <https://reviews.apache.org/r/73119/#comment311460> Does use of second scan result in significant performance improvements i.e. avoiding population of event.details and event.entity during the first scan? If not, it will be simper to move #320 - #326 to #293 and avoid this second scan. webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java Lines 802 (patched) <https://reviews.apache.org/r/73119/#comment311458> Why not add 'sortBy' query-param to existing REST API method, instead of adding a new method? Also, consider adding 'sortOrder' query-param - with default value as 'asc'. - Madhan Neethiraj On Jan. 6, 2021, 7:06 p.m., Deep Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73119/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2021, 7:06 p.m.) > > > Review request for atlas, Ashutosh Mestry, Madhan Neethiraj, Sarath > Subramanian, and Sidharth Mishra. > > > Bugs: ATLAS-4094 > https://issues.apache.org/jira/browse/ATLAS-4094 > > > Repository: atlas > > > Description > ------- > > Created a new entity audit API with Sorting functionality. > > /api/atlas/v2/entity/{entity-guid}/audit/sortby/{sort-column}?offset=2&count=5 > > sort-column may have 3 values "user", "action", or "timestamp". > > HBase does not support query with sorted results. To support this API > inmemory sort has to be performed. > Audit entry can potentially have entire entity dumped into it. Loading entire > audit entries for an entity can be memory intensive. Therefore we load audit > entries with limited columns first, perform sort on this light weight list, > then get the relevant section by removing offsets and reducing to limits. > With this reduced list we create MultiRowRangeFilter and then again scan the > table to get all the columns from the table this time. > > > Diffs > ----- > > > repository/src/main/java/org/apache/atlas/repository/audit/CassandraBasedAuditRepository.java > 8a453fd43 > > repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditRepository.java > 07784d1c4 > > repository/src/main/java/org/apache/atlas/repository/audit/HBaseBasedAuditRepository.java > 9fca74470 > > repository/src/main/java/org/apache/atlas/repository/audit/InMemoryEntityAuditRepository.java > 900df0205 > > repository/src/main/java/org/apache/atlas/repository/audit/NoopEntityAuditRepository.java > ef9e259ea > webapp/src/main/java/org/apache/atlas/web/rest/EntityREST.java 0d6d0c845 > > > Diff: https://reviews.apache.org/r/73119/diff/2/ > > > Testing > ------- > > Manual testing was done. > > > Thanks, > > Deep Singh > >
