----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18938/#review36597 -----------------------------------------------------------
Thank you very much for this work!! I have some minor comments below but otherwise I think it looks great! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/18938/#comment67618> Same concern about modifying the set while iterating over it as below. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java <https://reviews.apache.org/r/18938/#comment67617> I am not sure we should modify this set while we are iterating over it? We could store the privilege in a temp variable and then modify it after the loop. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java <https://reviews.apache.org/r/18938/#comment67616> Feels strange that this one throws an exception but removeRole does not? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/18938/#comment67621> Same could be said about append* methods as the remove* methods. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/18938/#comment67620> I believe a common way to handle this is to have the remove methods perform the fix-up. That is you have removePrivilege method also call removeRole and removeRole also call removePrivilege. You need logic that stops the recursion (when either the role or privilege is not found) but it makes it much nicer for users of the objects. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java <https://reviews.apache.org/r/18938/#comment67615> since tables can contain _ how about we use space, comma, or something else? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java <https://reviews.apache.org/r/18938/#comment67613> I don't think Java "assert" should not be used here because it requires assertions to be enabled. I think this statement should be: assertTrue("Unexpected status: " + statusCode, statusCode == ....); Also is ALREADY_EXISTS valid for DropSentryRoleRequest? I think it should be NO_SUCH_OBJECT. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java <https://reviews.apache.org/r/18938/#comment67614> same assert() issue here - Brock Noland On March 8, 2014, 2:12 a.m., Shreepadma Venugopalan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18938/ > ----------------------------------------------------------- > > (Updated March 8, 2014, 2:12 a.m.) > > > Review request for sentry, Brock Noland and Prasad Mujumdar. > > > Bugs: SENTRY-126 > https://issues.apache.org/jira/browse/SENTRY-126 > > > Repository: sentry > > > Description > ------- > > This patch, > > a) implements apis needed to grant and revoke privileges to roles > b) adds a new test case that passes > c) cleans up the thrift interface > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/SentryPolicyService.java > 04e6655 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TAlterSentryRoleAddGroupsRequest.java > 034bfef > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TAlterSentryRoleGrantPrivilegeRequest.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TAlterSentryRoleGrantPrivilegeResponse.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TAlterSentryRoleRevokePrivilegeRequest.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TAlterSentryRoleRevokePrivilegeResponse.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TCreateSentryPrivilegeRequest.java > 34689fc > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TCreateSentryPrivilegeResponse.java > a92698e > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryRolesResponse.java > 32370ba > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java > 37f9fb7 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/service/thrift/sentry_common_serviceConstants.java > 8174fe2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > 549a9db > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 28416e0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryInvalidInputException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 7419a0d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandler.java > dbd8dae > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/NotificationHandlerInvoker.java > 2425ac3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > aa83ffd > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 55dbcdd > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Status.java > 0b2daf3 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_common_service.thrift > ed0ebc5 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > 40f8a5f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestNotificationHandlerInvoker.java > dab26e1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > 692dbfa > > Diff: https://reviews.apache.org/r/18938/diff/ > > > Testing > ------- > > New test case passes. Existing tests pass. > > > Thanks, > > Shreepadma Venugopalan > >
