----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24967/#review61833 -----------------------------------------------------------
I am still a bit skepticle about using the combinitions of fields to determin version mismatch. It's hard to distinguish the old client vs new client setting wrong fields. Note that Sentry service has non java clients like Hue that use the raw thrift calls and no the sentry client methods. IMO we should increment the rpc version, and use the version to detect the invalid field settings. Any case, I am fine with doing this in a followup patch. The current code should work fine with new client and new server combinition. - Prasad Mujumdar On Nov. 11, 2014, 3:13 p.m., Dapeng Sun wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24967/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2014, 3:13 p.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/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 > 9f16b73 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f6699d2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 39371b7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > b20e71e > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > 7e6ade5 > > 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 > 2e829d6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > 50ee559 > > Diff: https://reviews.apache.org/r/24967/diff/ > > > Testing > ------- > > Unit Tests in local > > > Thanks, > > Dapeng Sun > >
