----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59655/#review176496 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1714 (original), 1714 (patched) <https://reviews.apache.org/r/59655/#comment249860> Note that in the code below you only use role names and never the roles thesevles, and role copy may be expensive (e.g. it can contain multiple privileges). It may be better (and simpler) to just collect a set of role names rather then a set of role copies. Also, on the left side of the assignment you can simply use Collection since you are only walking it. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1715 (original), 1715 (patched) <https://reviews.apache.org/r/59655/#comment249861> This can be moved to the place below where parent is actually used and combined with assignment. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1717 (original), 1717 (patched) <https://reviews.apache.org/r/59655/#comment249863> mPrivileges can never be null and there is no need to check for empty in the loop below so this can be simplified to for (MSentryPrivilege mPrivilege : mPrivileges) { roleSet.addAll(ImmutableSet.copyOf(mPrivilege.getRoles())); } sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1718 (original), 1718 (patched) <https://reviews.apache.org/r/59655/#comment249865> If we use String role names we can do something like Collection<String> roleNames = new HashSet<>(); List<MSentryPrivilege> mPrivileges = getMSentryPrivileges(tPrivilege, pm); for (MSentryPrivilege mPrivilege : mPrivileges) { roleNames.addAll(rolesToRoleNames(mPrivilege.getRoles())); } sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1723 (original), 1723 (patched) <https://reviews.apache.org/r/59655/#comment249866> Space after // sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 1730 (patched) <https://reviews.apache.org/r/59655/#comment249867> Space after // sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1726 (original), 1734 (patched) <https://reviews.apache.org/r/59655/#comment249881> The left side can be Collection instead of Set sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1728 (original), 1736 (patched) <https://reviews.apache.org/r/59655/#comment249872> Space after // Please explain why parent may be removed inside this transaction. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1729 (original), 1737 (patched) <https://reviews.apache.org/r/59655/#comment249874> Whe the associated row in the database is removed sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1730 (original), 1738 (patched) <https://reviews.apache.org/r/59655/#comment249873> dereferenced sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1731 (original), 1739 (patched) <https://reviews.apache.org/r/59655/#comment249864> Do we need a fresh parent copy for every loop iteration or we can do it once before the loop starts? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1734 (original), 1741 (patched) <https://reviews.apache.org/r/59655/#comment249880> Here and below instead of using newHashSet, use singleton(role.getRoleName) - see https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#singleton(T) sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1736 (original), 1743 (patched) <https://reviews.apache.org/r/59655/#comment249875> Do you need a new instance of MPrivilege on every iteration? The tPrivilege never changes in the loop, so there is no need to convert it on every loop iteration - it can be done upfront. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1743 (original), 1749 (patched) <https://reviews.apache.org/r/59655/#comment249882> m is a rather bad name for privilege sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1744 (original), 1750 (patched) <https://reviews.apache.org/r/59655/#comment249883> t is also not the best name for privilege sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 1745 (original), 1751 (patched) <https://reviews.apache.org/r/59655/#comment249884> Here and below, newTPrivilege never changes, so we can pre-compute everything upfront and simplify the code below. - Alexander Kolbasov On May 30, 2017, 11:17 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59655/ > ----------------------------------------------------------- > > (Updated May 30, 2017, 11:17 p.m.) > > > Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, > Sergio Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1788 > https://issues.apache.org/jira/browse/SENTRY-1788 > > > Repository: sentry > > > Description > ------- > > Root Cause: Application was using JDO object even after the associated > database entry was deleted. > Fix: Made code change so that JDO object is detached so that deletion of data > in the database would not invalidate the object used by the application. > parent object which is an JDO object used in > SentryStore.dropOrRenamePrivilegeForAllRoles after associated data in the > database is deleted. Method alterSentryRoleRevokePrivilegeCore which called > would internally delete the data from database.As part of this exercise > > Summary of the code changes > 1. Fix as described above > 2. Code optimization. dropOrRenamePrivilegeForAllRoles method handles both > drop and rename of privileges. There is certain logic which constructs > privilege graph which is executed for both dropping and renaming of the > privilege. This logic needs to be executed only for renaming of privileges > as the privilege graph constructed is used only when the privilege is renamed. > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 958a46c > > > Diff: https://reviews.apache.org/r/59655/diff/1/ > > > Testing > ------- > > Made sure the unit tests are passing. > > > Thanks, > > kalyan kumar kalvagadda > >
