----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67648/#review205014 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 796 (original), 796 (patched) <https://reviews.apache.org/r/67648/#comment287865> Should it be 'User does not exist'? Might sounds that it can exist. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 1123-1127 (original), 1123-1128 (patched) <https://reviews.apache.org/r/67648/#comment287866> If the mEntity is null, then there is nothing to remove, so there is nothing to persist. The 2nd if() will call persistPrivilege() no matter if the mEntity is null or not. I read the previous code and it had something like this: if (!persistedPriv.getRoles().isEmpty()) { persistedPriv.removeRole(mRole); if (persistedPriv.getRoles().isEmpty()) { pm.deletePersistent(persistedPriv); } else { pm.makePersistent(persistedPriv); } } The above code makese sense because if it removes a role, then it will either delete the privilege or just update the privilege. Why was the deletePersisten() removed previusly? Shoud we update the privilege only if it removed an entity? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2517 (original), 2518 (patched) <https://reviews.apache.org/r/67648/#comment287868> Update the commet that this will drop the privilege for all entities (roles and users) sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2527 (original), 2528 (patched) <https://reviews.apache.org/r/67648/#comment287869> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2534 (original), 2535 (patched) <https://reviews.apache.org/r/67648/#comment287867> Should we name thie method 'dropPrivilegeForAllEntities'? It makes more sense what it is meant for. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2548 (original), 2549 (patched) <https://reviews.apache.org/r/67648/#comment287870> Update the comment that it is for all the entities sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2560 (original), 2561 (patched) <https://reviews.apache.org/r/67648/#comment287874> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2581 (original), 2582 (patched) <https://reviews.apache.org/r/67648/#comment287875> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2594 (original), 2595 (patched) <https://reviews.apache.org/r/67648/#comment287876> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2618 (original), 2619 (patched) <https://reviews.apache.org/r/67648/#comment287877> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2635 (original), 2636 (patched) <https://reviews.apache.org/r/67648/#comment287878> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2664 (original), 2665 (patched) <https://reviews.apache.org/r/67648/#comment287879> should be renamePrivilegeForAllEntities? Same for the other methods that saye ForAll only. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2672 (original), 2673 (patched) <https://reviews.apache.org/r/67648/#comment287880> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2684 (original), 2685 (patched) <https://reviews.apache.org/r/67648/#comment287881> Update the comment that it is for all entities. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2709 (original), 2711 (patched) <https://reviews.apache.org/r/67648/#comment287882> Update the comment that it is for all entities. - Sergio Pena On June 19, 2018, 3:37 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67648/ > ----------------------------------------------------------- > > (Updated June 19, 2018, 3:37 p.m.) > > > Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena. > > > Bugs: SENTRY-2251 > https://issues.apache.org/jira/browse/SENTRY-2251 > > > Repository: sentry > > > Description > ------- > > User privileges granted to user on tables/databases should be removed when > the authorizable is dropped. > User privileges granted to user on tables/databases should be updated when > the authorizable are renamed. > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f0e373a9aa5342d1b507e8b192cfdbc444242227 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > c056446262ddcf61db307eafbb785eac42973c80 > > > Diff: https://reviews.apache.org/r/67648/diff/1/ > > > Testing > ------- > > Made sure all the existing tests pass. Also updated tests to cover new use > cases added by this patch. > > > Thanks, > > kalyan kumar kalvagadda > >
