> On July 22, 2014, 2:52 p.m., Arun Suresh wrote: > > Xiomeng, thank you for working on this patch !! > > > > Along with my other comments, I was wondering if you could remove the > > dependency of "PRIVILEGE_NAME" from your solution. Since the field itself > > is just a composite of Server, Db, Table, URI and action. This can be > > modeled as a aggregate key. Please take a look at SENTRY-339
Hi, Arun Thanks for your review! I have seen SENTRY-339. It's very good. I will update my patch depends on yours > On July 22, 2014, 2:52 p.m., Arun Suresh wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, > > line 55 > > <https://reviews.apache.org/r/23786/diff/3/?file=638815#file638815line55> > > > > grantOption can be a Boolean i guess Hi, this is a good point. I explain it in my design doc. 1.In my design, grant option support priority. High priority can grant/revoke low priority. 2.For solving hive revoke privilege without "grant option" issue, we gave grantOption=-1 a special meaning. If grantOption=-1, we will revoke all privileges with same privilegeName. > On July 22, 2014, 2:52 p.m., Arun Suresh wrote: > > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql, > > line 83 > > <https://reviews.apache.org/r/23786/diff/3/?file=638817#file638817line83> > > > > Any particular reason you need "WITH_GRANT_OPTION" as part of your > > index ? In database, it may have two privilege with same privilegeName and different grantOption. e.g. user1 has privilege {privilegeName=server1+db1+tb1+select, grantOption=1}. user1 can grant this privilege to other role. user2 has privilege {privilegeName=server1+db1+tb1+select, grantOption=0}. user2 can't grant this privilege to other role. > On July 22, 2014, 2:52 p.m., Arun Suresh wrote: > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.4.0.sql, > > line 29 > > <https://reviews.apache.org/r/23786/diff/3/?file=638818#file638818line29> > > > > Can we use BIT/BOOLEAN datatype since this value is either 0 or 1 I explain it in first comment. SentryStore is general for other components, and not only for hive. In database design, we support it with priority. And value "-1" has a special meaning. - Xiaomeng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23786/#review48363 ----------------------------------------------------------- On July 22, 2014, 6:29 a.m., Xiaomeng Huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23786/ > ----------------------------------------------------------- > > (Updated July 22, 2014, 6:29 a.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > This is database design and implement to support "with grant option". > Add a field grantOption(int) to table SENTRY_DB_PRIVILEGE. > And modify the table SENTRY_DB_PRIVILEGE from a single index {privilegeName} > to a composite index with member {privilegeName, grantOption} > > > Diffs > ----- > > > 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/package.jdo > 945227e > 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/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 7637376 > > Diff: https://reviews.apache.org/r/23786/diff/ > > > Testing > ------- > > > Thanks, > > Xiaomeng Huang > >
