You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Xiaomeng Huang <xi...@intel.com> on 2014/08/22 07:40:51 UTC
Review Request 24961: SENTRY-391: Extend sentrystore query for column
level privilege
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24961/
-----------------------------------------------------------
Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya Tirukkovalur.
Repository: sentry
Description
-------
rename table should get child privileges firstly, then add new child privileges with new tablename
Diffs
-----
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java de35bfa
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/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 33600e9
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/service/persistent/TestSentryPrivilege.java 91d3171
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 985a73d
Diff: https://reviews.apache.org/r/24961/diff/
Testing
-------
test case are included.
Thanks,
Xiaomeng Huang
Re: Review Request 24961: SENTRY-391: Extend sentrystore query for
column level privilege
Posted by Xiaomeng Huang <xi...@intel.com>.
> On Sept. 13, 2014, 1:28 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, line 276
> > <https://reviews.apache.org/r/24961/diff/2/?file=676637#file676637line276>
> >
> > Could you please add some more test cases on partial privileges, eg remove insert from all. That part of code is bit messy, so it would be good to have a bit more tests for that. Thanks!
I have test cases to handle partial privileges, pls take a look at TestSentryStore.testGrantRevokePrivilegeWithColumn()
> On Sept. 13, 2014, 1:28 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 545
> > <https://reviews.apache.org/r/24961/diff/2/?file=676634#file676634line545>
> >
> > I guess if this is a server level privilege, then both will be null. Do we have to add that check as well ?
Yes, I will fix it.
> On Sept. 13, 2014, 1:28 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift, line 48
> > <https://reviews.apache.org/r/24961/diff/2/?file=676635#file676635line48>
> >
> > I don't think we should change the sequence of fields. Thrift use that to maintain the backward compatibility.
> > The columnName should be added at the end as 11th field.
Yes, if we just upgrade the sentry server library, it will has compartible issues with client side in thrift. I will fix it.
- Xiaomeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24961/#review53257
-----------------------------------------------------------
On Sept. 4, 2014, 10:23 a.m., Xiaomeng Huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24961/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2014, 10:23 a.m.)
>
>
> Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> rename table should get child privileges firstly, then add new child privileges with new tablename
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java de35bfa
> 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/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 33600e9
> 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/service/persistent/TestSentryPrivilege.java 91d3171
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 985a73d
>
> Diff: https://reviews.apache.org/r/24961/diff/
>
>
> Testing
> -------
>
> test case are included.
>
>
> Thanks,
>
> Xiaomeng Huang
>
>
Re: Review Request 24961: SENTRY-391: Extend sentrystore query for
column level privilege
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24961/#review53257
-----------------------------------------------------------
Looks mostly fine. A few of minor comments below.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/24961/#comment92839>
I guess if this is a server level privilege, then both will be null. Do we have to add that check as well ?
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/24961/#comment92840>
I don't think we should change the sequence of fields. Thrift use that to maintain the backward compatibility.
The columnName should be added at the end as 11th field.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
<https://reviews.apache.org/r/24961/#comment92841>
Could you please add some more test cases on partial privileges, eg remove insert from all. That part of code is bit messy, so it would be good to have a bit more tests for that. Thanks!
- Prasad Mujumdar
On Sept. 4, 2014, 10:23 a.m., Xiaomeng Huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24961/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2014, 10:23 a.m.)
>
>
> Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> rename table should get child privileges firstly, then add new child privileges with new tablename
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java de35bfa
> 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/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 33600e9
> 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/service/persistent/TestSentryPrivilege.java 91d3171
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 985a73d
>
> Diff: https://reviews.apache.org/r/24961/diff/
>
>
> Testing
> -------
>
> test case are included.
>
>
> Thanks,
>
> Xiaomeng Huang
>
>
Re: Review Request 24961: SENTRY-391: Extend sentrystore query for
column level privilege
Posted by Xiaomeng Huang <xi...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24961/
-----------------------------------------------------------
(Updated Sept. 4, 2014, 10:23 a.m.)
Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
fix a bug of rename/drop privilege if this privilege is not exsit in db, but we should rename/drop the child privileges of it. Also add test cases to test it.
Repository: sentry
Description
-------
rename table should get child privileges firstly, then add new child privileges with new tablename
Diffs (updated)
-----
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizable.java de35bfa
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/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 33600e9
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/service/persistent/TestSentryPrivilege.java 91d3171
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 985a73d
Diff: https://reviews.apache.org/r/24961/diff/
Testing
-------
test case are included.
Thanks,
Xiaomeng Huang