> On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java, > > line 20 > > <https://reviews.apache.org/r/24151/diff/1/?file=646824#file646824line20> > > > > Spelling of the class: SentryNoGrantOpitonException => > > SentryNoGrantOptionException > > > > Also, SentryAccessDeniedException is mostly used when user does is not > > part of sentry admin group, but is trying to create role, grant role or > > grant privilege. Although I see that it is also used else where, there is > > scope for some clean up. For example, SentryStore.getMSentryVersion() > > (Filed SENTRY-378 to clean up). But after this patch, looks like they both > > (accessdenied and nograntoption) mean more or less the same exception. We > > can follow up on this as a follow on. > > > > Sun Dapeng wrote: > Yes, this will be fixed.
In my opinion, SentryAccessDeniedException is meaning access data denied, and NoGrantOption is grant privilege denied. I want to change the name of the exception to SentryGrantDeniedException. And SentryGrantDeniedException will be parallel with SentryAccessDeniedException, both extends SentryUserException. This is just my thoughts, if you think we can reuse the SentryAccessDeniedException to instead of grantoption exception. It's OK on my side, I also think it's simple and clearly. > On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, > > lines 234-237 > > <https://reviews.apache.org/r/24151/diff/1/?file=646825#file646825line234> > > > > URI is a child of server, so we need to check serverName equality even > > when uris are same. fixed > On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, > > line 239 > > <https://reviews.apache.org/r/24151/diff/1/?file=646825#file646825line239> > > > > Minor - Feel free to ignore: > > In practice serverName can never be null. So may be it is better to > > return false even if both server names are null? agreed. fixed > On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 468 > > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line468> > > > > I am not exactly sure why need this? I might be missing some thing. fixed. > On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 492 > > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line492> > > > > would'nt the table name always __NULL__ here as we have a check earlier? agreed and fixed. Refactor revoke with grantOption > On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1366 > > <https://reviews.apache.org/r/24151/diff/1/?file=646828#file646828line1366> > > > > Performance: getRoleNamesForGroups is building rolenames from > > msentryroles, and again we are building msentryrole from role name here. So > > there is chance for performance improvement if we create a new function > > which returns Set<MSentryRoles> instead? It's a good comment. fixed. thx. > On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryPrivilege.java, > > line 56 > > <https://reviews.apache.org/r/24151/diff/1/?file=646838#file646838line56> > > > > Minor: may be just use all? As we do not support select on uri? fixed > On July 31, 2014, 10:47 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, > > line 367 > > <https://reviews.apache.org/r/24151/diff/1/?file=646839#file646839line367> > > > > Nit: Change comment to "insert" ? fixed - Xiaomeng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24151/#review49253 ----------------------------------------------------------- On July 31, 2014, 11:16 a.m., Sun Dapeng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24151/ > ----------------------------------------------------------- > > (Updated July 31, 2014, 11:16 a.m.) > > > Review request for sentry, Arun Suresh, Jarek Cecho, Prasad Mujumdar, and > Sravya Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > This review request contains all 8 subtasks(need apply the patch of > SENTRY-339 first) > SENTRY-340 Database implement for "with grant option" > SENTRY-341 Extend Thrift API for SentryStore to support "with grant option" > SENTRY-343 Privileges query from database support for "With Grant Option" > SENTRY-370 Judgement of MSentryPrivilege implies child privileges > SENTRY-342 Grant check with grant option > SENTRY-345 Revoke check with grant option > SENTRY-349 Extend Hive Hook with Grant Option > SENTRY-377 Add Hive e2e test for grantOption > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/SentryHiveConstants.java > 26eea91 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 27a10ee > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java > 991d734 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryHiveAuthorizationTaskFactory.java > ac0d170 > sentry-core/sentry-core-common/pom.xml d1785b8 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java > 962179f > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/DBWildcardPrivilege.java > 896283c > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryGrantOption.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/gen/thrift/gen-javabean/org/apache/sentry/provider/db/service/thrift/TSentryPrivilege.java > 9e8ac4c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryNoGrantOpitonException.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > f8491db > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java.orig > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 945227e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > ff8acdc > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java.orig > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 5fd4f8f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 3bb7285 > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql > f2a62d2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.4.0.sql > f2a62d2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.4.0.sql > 70f4dbb > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.4.0.sql > 363590e > > sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.4.0.sql > 5dfae03 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > fdc7b9c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryPrivilege.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 7637376 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > a4ae291 > sentry-provider/sentry-provider-file/pom.xml b834c8e > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestPrivilegeWithGrantOption.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/24151/diff/ > > > Testing > ------- > > Unit tests in local > > > Thanks, > > Sun Dapeng > >
