----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67452/#review204340 -----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 865 (patched) <https://reviews.apache.org/r/67452/#comment286802> There exists a alterSentryRoleGrantPrivilegeCore() and alterSentryUserGrantPrivilegeCore() methods already. is there a need to have this method? Can we have an if() section in the caller and make the right call instead? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2285 (patched) <https://reviews.apache.org/r/67452/#comment286801> This method looks pretty similar to getMSentryPrivilegesByAuth() except that it sets the OWNER as action. Isn't better to add the action as parameter and reuse the other method instead? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2982-2989 (patched) <https://reviews.apache.org/r/67452/#comment286781> This comment is orphan or out of place. I see two header comments in this method. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3000 (patched) <https://reviews.apache.org/r/67452/#comment286790> Shouldn't we search for the current owner privileges of the tAuthorizable here and delete those instead of passing them as a parameter? Being a public api this could be prone to errors if we expect the caller should pass those privileges to revoke. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3016 (patched) <https://reviews.apache.org/r/67452/#comment286793> There should be a better way to delete privileges in one call instead of one at a time. Like DELETE FROM mPrivilege WHERE type = OWNER & db = db1 && table = t1, right? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3020 (patched) <https://reviews.apache.org/r/67452/#comment286795> We should check what info this object brings. Owner privileges can only be granted to databases and tables. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3031-3032 (patched) <https://reviews.apache.org/r/67452/#comment286797> Is it only for dropping privileges? I think the message is incorrect here. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 4953 (patched) <https://reviews.apache.org/r/67452/#comment286785> Why is the number 3 needed? Theexecute() method for a single update has 2 because it only adds up to to updates, but what about here? does it need a comment on why 3 is a good number? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 4954-4955 (patched) <https://reviews.apache.org/r/67452/#comment286786> Our coding convention is to have spaces between (), like if () and for (). Also a space between the :, like update : updates - Sergio Pena On June 5, 2018, 7:34 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67452/ > ----------------------------------------------------------- > > (Updated June 5, 2018, 7:34 p.m.) > > > Review request for sentry, Na Li and Sergio Pena. > > > Bugs: SENTRY-2257 > https://issues.apache.org/jira/browse/SENTRY-2257 > > > Repository: sentry > > > Description > ------- > > Implement functionality in sentry store to update owner privilege on an > authorizable. > > Here is the approach. > > There are two new API's that are exposed. > > To list the owner privileges granted to an authorizable > 1. update the owner privilege to new owner > > Here is the Flow. > 1. SentryPolicyStoreProcessor would first get the list of privileges that are > to be revoked. > 2. Using the list of privileges that are to be revoked, list of > PermissionsUpdate is generated using SentryPlug-in > 3. SentryPolicyStoreProcessor would then use the new API to update the owner > privileges. > > This way all the updated listed below happen in the same transaction > 1. Revoking the exixting owner privilage for authorizable > 2. Granting new owner privilege fot authorizable. > 3. Adding delta update for owner privilege revoked > 4. Adding delta update for owner privilege granted. > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 5932335ef9f6e3f894da9a65a4bf1bdedcbe0ffc > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > e2d24e53d0d3f8a738066cfedc85b11f6535f45c > > > Diff: https://reviews.apache.org/r/67452/diff/3/ > > > Testing > ------- > > Added new tests to verify new functionality added. > > > Thanks, > > kalyan kumar kalvagadda > >
