----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24967/#review55467 -----------------------------------------------------------
Looks fine. Just one comment below. Also you'll need to rebase the patch since the thrift interface has other change. Please regenerate the thirft java code after rebase. sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/24967/#comment95843> This will make the older clients incompatible with new service. We should change the protocol version to V2 so that the older client can get a more meaningful error. - Prasad Mujumdar On Sept. 9, 2014, 10:15 a.m., Sun Dapeng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24967/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2014, 10:15 a.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > The patch include: > * SENTRY Thrift API changed : > 1. We change the field {{TSentryPrivilege privilege}} to > {{set<TSentryPrivilege> privileges}} in > {{TAlterSentryRoleGrantPrivilegeRequest}} and > {{TAlterSentryRoleRevokePrivilegeRequest}}, The reason is the HIVE GRANT may > like {{Grant SELECT (tb1.col1, tb2.col2) on TABLE table1 to role roleName}}, > it contains two privileges ({{col1}} and {{col2}}) for SENTRY, to reduce the > request API calls, we make it change. > > 2. Another way to Implement it, maybe add a {{column list}} to > {{TSentryPrivilege}}, but it will bring more problems, we know SentryStore > has many convert methods between {{TSentryPrivilege}} and > {{MSentryPrivilege}}, and query an unique {{MSentryPrivilege}} use > {{TSentryPrivilege}} as query condition, so we should make them one-to-one > correspondence. > > * Change {{SentryStore}} after Thrift API changed > > * Change {{SentryPolicyStoreProcessor}} and {{SentryPolicyServiceClient}} > after Thrift API changed, include the grant/revoke methods about column > privilege > > * Change {{Auditlog}} after Thrift API changed > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TAlterSentryRoleGrantPrivilegeRequest.java > 62b6b31 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TAlterSentryRoleRevokePrivilegeRequest.java > bbd9536 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForProviderRequest.java > 10ab56b > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesForProviderResponse.java > 4c571c2 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryPrivilegesResponse.java > d34205a > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TListSentryRolesResponse.java > 13f22ff > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryActiveRoleSet.java > 573dc26 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryRole.java > f43a6d5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/AuditMetadataLogEntity.java > e1d8a9e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/entity/JsonLogEntityFactory.java > 2cc8194 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/util/CommandUtil.java > 841eeb3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/log/util/Constants.java > 4b1d7de > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 718306d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 6895927 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 070c494 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > b14616b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestAuditMetadataLogEntity.java > cd0a435 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/entity/TestJsonLogEntityFactory.java > fc9c716 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/log/util/TestCommandUtil.java > 5da8591 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 985a73d > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > 5244094 > > Diff: https://reviews.apache.org/r/24967/diff/ > > > Testing > ------- > > Unit Tests in local > > > Thanks, > > Sun Dapeng > >
