> On 八月 26, 2014, 9:22 a.m., Sravya Tirukkovalur wrote: > > Nice work! Mostly looks good to me. Minor comments below.
Sravya, thanks very much for your review and comment! > On 八月 26, 2014, 9:22 a.m., Sravya Tirukkovalur wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestExportImportPrivileges.java, > > line 107 > > <https://reviews.apache.org/r/24973/diff/1/?file=667151#file667151line107> > > > > Looks like we do not have a use case in Hive where we will need insert > > privileges at Column scope. If so, can we notify users trying to do "GRANT > > INSERT on (c1,c2) on TABLE tb1 to .." that it is not supported? Fixed. Thanks! > On 八月 26, 2014, 9:22 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift, > > line 109 > > <https://reviews.apache.org/r/24973/diff/1/?file=667138#file667138line109> > > > > As this also allows to add mutliple privileges at table/db scope. Can > > we add tests for that? > > Xiaomeng Huang wrote: > For multiple columns, we extend the thrift with a set of privileges. So > we support multiple privileges in backend, multiple table/database is ok, too. > But hive grant/revoke command don't support multiple table/database, do > you think it is necessary to add test cases for multiple table/database? Hi, Sravya, like Xiaomeng's comment. the methods grantPrivilege/revokePrivilege do the converting from HIVE privileges to TAlterSentryRoleGrantPrivilegeRequest, no method can do "add mutliple privileges at table/db scope" in SentryGrantRevokeTask, so the UnitTest can be only added to TestSentryStore, we will add new tests in TestSentryStore, thanks! > On 八月 26, 2014, 9:22 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > lines 1333-1349 > > <https://reviews.apache.org/r/24973/diff/1/?file=667130#file667130line1333> > > > > If any comment here? Please let me know. - Sun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24973/#review51447 ----------------------------------------------------------- On 八月 22, 2014, 6:12 p.m., Sun Dapeng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24973/ > ----------------------------------------------------------- > > (Updated 八月 22, 2014, 6:12 p.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > The jira include 6 SENTRY subtask and 1 HIVE jira: > > 1. SENTRY-389 Database implementation for column > 2. SENTRY-391 Extend sentrystore query for column level privilege > 3. SENTRY-390 Extend Thrift API to support column-level privilege > 4. SENTRY-393 Grant/Revoke and Show Grant info support for column level > privilege > 5. HIVE-7730 Extend ReadEntity to add accessed columns from query > (https://reviews.apache.org/r/24962/) > 6. SENTRY-392 Authorization for column level security > 7. SENTRY-394 PolicyFile and ConfigImport support for column level privilege > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java > 49922f9 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 0b26806 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryHivePrivilegeObjectDesc.java > 1b5f557 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 2df741c > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > f38ee91 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java > 761082a > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > 1c32946 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java > 6e40a5c > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Column.java > PRE-CREATION > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java > de35bfa > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBModelAuthorizables.java > 873f789 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > 51d4248 > > 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/TSentryAuthorizable.java > 59418a3 > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java > 54b6204 > > 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/model/MSentryPrivilege.java > 5328fff > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > b39cb18 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 33600e9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > d4c5806 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > f227a02 > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql > c1a2778 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.4.0.sql > c1a2778 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.4.0.sql > d7ce57c > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.4.0.sql > 3f01cda > > sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.4.0.sql > 186c968 > > 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/TestSentryPrivilege.java > 91d3171 > > 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/persistent/TestSentryStoreToAuthorizable.java > d77786a > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > 5244094 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java > 8b83859 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegesAtColumnScope.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 3a7aa41 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestConfigTool.java > c186a42 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestExportImportPrivileges.java > eaf3816 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java > c238361 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPrivilegesAtColumnScope.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java > 4c66ffe > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/SentryPolicyProviderForDb.java > c60d0d5 > > Diff: https://reviews.apache.org/r/24973/diff/ > > > Testing > ------- > > unit test in local > > > Thanks, > > Sun Dapeng > >
