You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arun Suresh <ar...@gmail.com> on 2014/07/17 03:47:12 UTC
Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/
-----------------------------------------------------------
Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
Repository: sentry
Description
-------
SENTRY-339 :
Remove the need to construct a privilegeName to check for uniqueness.
- Remove the PrivilegeName column
- Remove the constructPrivilegeName() method
- Updated the sql scripts
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/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/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/TestSentryStore.java 7637376
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
Diff: https://reviews.apache.org/r/23600/diff/
Testing
-------
Ran all e2e tests and unit tests.
Thanks,
Arun Suresh
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Brock Noland <br...@cloudera.com>.
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, lines 58-61
> > <https://reviews.apache.org/r/23600/diff/1/?file=634109#file634109line58>
> >
> > why are we replacing nulls with empty strings, is it because NULL != NULL in some dbs?
>
> Arun Suresh wrote:
> yup... some dbs can't handle null columns in composite indexes
Also note that oracle cannot store an empty string. It converts empty string to null.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48400
-----------------------------------------------------------
On July 24, 2014, 12:32 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 12:32 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, lines 58-61
> > <https://reviews.apache.org/r/23600/diff/1/?file=634109#file634109line58>
> >
> > why are we replacing nulls with empty strings, is it because NULL != NULL in some dbs?
>
> Arun Suresh wrote:
> yup... some dbs can't handle null columns in composite indexes
>
> Brock Noland wrote:
> Also note that oracle cannot store an empty string. It converts empty string to null.
Thanks for the catch Brock... yup looks like Oracle screws up empty strings... handling it in the next patch
- Arun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48400
-----------------------------------------------------------
On July 24, 2014, 9:41 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 9:41 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, line 57
> > <https://reviews.apache.org/r/23600/diff/1/?file=634109#file634109line57>
> >
> > I do not think serverName can ever be null?
Agreed... I just put it there for completeness
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, lines 58-61
> > <https://reviews.apache.org/r/23600/diff/1/?file=634109#file634109line58>
> >
> > why are we replacing nulls with empty strings, is it because NULL != NULL in some dbs?
yup... some dbs can't handle null columns in composite indexes
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift, line 36
> > <https://reviews.apache.org/r/23600/diff/1/?file=634118#file634118line36>
> >
> > We should probably not provide a default for serverName? We should make serverName strictly required.
so looks like there is something funny going on in our thrift version.. I am able to create a TSentryPrivilege with null serverName. This seemed to work for me finally
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.4.0.sql, line 84
> > <https://reviews.apache.org/r/23600/diff/1/?file=634115#file634115line84>
> >
> > This might lead to problem as URI(4000) is too big to be part of unique key.
will fix thanx..
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql, line 82
> > <https://reviews.apache.org/r/23600/diff/1/?file=634113#file634113line82>
> >
> > Wonder why are some in quotes? Are they system keywords? http://www-01.ibm.com/support/docview.wss?uid=swg21164500
looks like.. these columns had "" where they were declared.. so decided to copy as it is..
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, lines 1115-1116
> > <https://reviews.apache.org/r/23600/diff/1/?file=634111#file634111line1115>
> >
> > curious, why you had to change this? Not part of this patch but also the scope should be lower cased right?
there was a testcase that was setting uppercase Actions... Did not want to change the test case, also, I thought doing this here will enforce lowercase. our actions are supposed to be lowercase rite ?
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java, line 214
> > <https://reviews.apache.org/r/23600/diff/1/?file=634119#file634119line214>
> >
> > Nit: Can we remove this commented out line?
will do.. thnx
- Arun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48400
-----------------------------------------------------------
On July 17, 2014, 1:47 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 1:47 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> 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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, lines 1115-1116
> > <https://reviews.apache.org/r/23600/diff/1/?file=634111#file634111line1115>
> >
> > curious, why you had to change this? Not part of this patch but also the scope should be lower cased right?
>
> Arun Suresh wrote:
> there was a testcase that was setting uppercase Actions... Did not want to change the test case, also, I thought doing this here will enforce lowercase. our actions are supposed to be lowercase rite ?
Yes, I believe we do lower case everything except for usernames and groupnames.
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql, line 82
> > <https://reviews.apache.org/r/23600/diff/1/?file=634113#file634113line82>
> >
> > Wonder why are some in quotes? Are they system keywords? http://www-01.ibm.com/support/docview.wss?uid=swg21164500
>
> Arun Suresh wrote:
> looks like.. these columns had "" where they were declared.. so decided to copy as it is..
Ok, might be best to do a script test on all dbs. Can you make sure these changes work?
> On July 22, 2014, 8:18 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift, line 36
> > <https://reviews.apache.org/r/23600/diff/1/?file=634118#file634118line36>
> >
> > We should probably not provide a default for serverName? We should make serverName strictly required.
>
> Arun Suresh wrote:
> so looks like there is something funny going on in our thrift version.. I am able to create a TSentryPrivilege with null serverName. This seemed to work for me finally
Hmm, that does not seem right. We should enforce that serverName can never be null. Can you add a test case for it if possible?
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48400
-----------------------------------------------------------
On July 24, 2014, 12:32 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 12:32 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48400
-----------------------------------------------------------
Thanks for the patch Arun! Mostly looks good, some comments below.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment84961>
I do not think serverName can ever be null?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment84962>
why are we replacing nulls with empty strings, is it because NULL != NULL in some dbs?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/23600/#comment84980>
curious, why you had to change this? Not part of this patch but also the scope should be lower cased right?
sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql
<https://reviews.apache.org/r/23600/#comment84983>
Wonder why are some in quotes? Are they system keywords? http://www-01.ibm.com/support/docview.wss?uid=swg21164500
sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.4.0.sql
<https://reviews.apache.org/r/23600/#comment84985>
This might lead to problem as URI(4000) is too big to be part of unique key.
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/23600/#comment84992>
We should probably not provide a default for serverName? We should make serverName strictly required.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
<https://reviews.apache.org/r/23600/#comment84993>
Nit: Can we remove this commented out line?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
<https://reviews.apache.org/r/23600/#comment84995>
Nit: Remove debugging help?
- Sravya Tirukkovalur
On July 17, 2014, 1:47 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 1:47 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> 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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Xiaomeng Huang <xi...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48615
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85331>
Hi, I think there are some code style format issues in hashCode and equals.
There are tabs in these func.
And at the beginning of function, you should have two spaces.
- Xiaomeng Huang
On July 24, 2014, 12:32 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 12:32 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48883
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
<https://reviews.apache.org/r/23600/#comment85645>
Sorry, I did not notice this earlier. action field is also a required field, so does not make sense to have a default for a required field.
- Sravya Tirukkovalur
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48879
-----------------------------------------------------------
Ship it!
Ship It!
- Sravya Tirukkovalur
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48826
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85563>
Don't think that should really matter..
- Arun Suresh
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
> On July 25, 2014, 9:38 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1030
> > <https://reviews.apache.org/r/23600/diff/3-4/?file=641483#file641483line1030>
> >
> > Why is this changed back?
So.. this is diff between 2 versions of my diff... if was always "safeTrim".. and doing a "safeTrimLower" had caused 1 test case to fail. so removing it since i didn't want to touch the testcase..
> On July 25, 2014, 9:38 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1011
> > <https://reviews.apache.org/r/23600/diff/3-4/?file=641483#file641483line1011>
> >
> > Servername should never be null?
Hmmm... it doesn't imply that serverName can be null.. Im just doing a safety check (I do a fromNULLCol for all columns in the aggregated unique key.. so thought i might as well put it for serverName as well)
- Arun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48777
-----------------------------------------------------------
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On July 25, 2014, 9:38 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1011
> > <https://reviews.apache.org/r/23600/diff/3-4/?file=641483#file641483line1011>
> >
> > Servername should never be null?
>
> Arun Suresh wrote:
> Hmmm... it doesn't imply that serverName can be null.. Im just doing a safety check (I do a fromNULLCol for all columns in the aggregated unique key.. so thought i might as well put it for serverName as well)
ok, sounds fine.
> On July 25, 2014, 9:38 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1030
> > <https://reviews.apache.org/r/23600/diff/3-4/?file=641483#file641483line1030>
> >
> > Why is this changed back?
>
> Arun Suresh wrote:
> So.. this is diff between 2 versions of my diff... if was always "safeTrim".. and doing a "safeTrimLower" had caused 1 test case to fail. so removing it since i didn't want to touch the testcase..
Makes sense, will it be possible for you to create a follow on bug just to track this please? Btw, thanks for catching this.
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48777
-----------------------------------------------------------
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48777
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/23600/#comment85512>
Servername should never be null?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
<https://reviews.apache.org/r/23600/#comment85513>
Why is this changed back?
- Sravya Tirukkovalur
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, lines 41-45
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line41>
> >
> > Should these be initialized to __NULL__ instead?
Don't really think it is needed ...
1) an MSentryPrivilege object is always created from a TSentryPrivilege in the code (I think JDO requires you to have the empty constructor.. else we would always use the other constructor).. and I already do a "SentryStore.toNULLCol()" conversion for all the fields in that constructor.
2) The no arg constructor is used only when JDO populates a MSentryPrivilege object from the Db. I think I have handled that in all places in the code... else most of our e2e test cases would fail ?
Come to think of it... I feel it should be kept as "". I initially intended this to be an empty string.. and this is what our public interfaces (thrift files etc. say it is) Ideally JDO should do the job of handling the empty <-> null string issues with Oracle.. but unfortunatelty, since we are forced to, we should contain that change to just the places we need it (right before we write/read to/from Db.. conversion from TSentryPRivilege <-> MSentryPrivilege etc.)
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, line 74
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line74>
> >
> > SentryStore.toNULLCOL ?
Same reason as above.. (We never really use the setters in the code... it is used only by JDO and we ensure that the __NULL__ to empty string conversion happens properly. things will be fine)
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, line 82
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line82>
> >
> > SentryStore.toNULLCOL ?
Same reason as above..
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, line 90
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line90>
> >
> > SentryStore.toNULLCOL ?
Same reason as above..
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, line 98
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line98>
> >
> > SentryStore.toNULLCOL ?
Same reason as above..
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, line 106
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line106>
> >
> > SentryStore.toNULLCOL ?
Same reason as above..
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, lines 157-158
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line157>
> >
> > Shall we use fromNULLCOL() here, as __NULL__ is more like reserved word for db purposes?
These will probably be displayed only in log messages.. and that too Debug ones.. Don't think it should matter
MSentryPrivilege is not supposed to be a public class.
- Arun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48781
-----------------------------------------------------------
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On July 25, 2014, 11:09 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java, lines 41-45
> > <https://reviews.apache.org/r/23600/diff/4/?file=642458#file642458line41>
> >
> > Should these be initialized to __NULL__ instead?
>
> Arun Suresh wrote:
> Don't really think it is needed ...
>
> 1) an MSentryPrivilege object is always created from a TSentryPrivilege in the code (I think JDO requires you to have the empty constructor.. else we would always use the other constructor).. and I already do a "SentryStore.toNULLCol()" conversion for all the fields in that constructor.
> 2) The no arg constructor is used only when JDO populates a MSentryPrivilege object from the Db. I think I have handled that in all places in the code... else most of our e2e test cases would fail ?
>
> Come to think of it... I feel it should be kept as "". I initially intended this to be an empty string.. and this is what our public interfaces (thrift files etc. say it is) Ideally JDO should do the job of handling the empty <-> null string issues with Oracle.. but unfortunatelty, since we are forced to, we should contain that change to just the places we need it (right before we write/read to/from Db.. conversion from TSentryPRivilege <-> MSentryPrivilege etc.)
Lets take a step back, so here are the reasons we need to handle null(__NULL__) (Only in MSentryPrivilege):
1. Unique composite keys cannot have null values and but want unique constraint on MPrivilege(server,db,table,uri, action) where db,table and uri can be nulls/empty strings.
2. There are some dbs (oracle) which auto convert "" to null value.
So ideally we should handle this nuance in MSentryPrivilege and keep it transparent to rest of the application is possible. But as we are doing most of the MSentry to TSentry conversions in SentryStore I see that we put that handling in SentryStore, which is fine. But we also need to handle new creations of MSentryPrivilege (both default and non default), because when we create new privileges we do not necessarily convert directly from TSentry (thrift object).
For example if I have a new feature where I need to do: (we do similar thing in revokePartialPrivilege)
MSentryPrivilege mSentryPrivilege = new MSentryPrivilege();
mSentryPrivilege.setServerName(serverName);
mSentryPrivilege.setDbName(dbName); //if db is null, it will lead to a "" string in db, and will be a problem with oracle.
But I do not see a reason, how having these values to be initialized as "" strings in MSentryPrivilege can help in any case when our contract is to always store __NULL__ in place of null or empty in the db for privilege table. Let me know if I am missing some case here.
Although, I see that current changes do not cause regression in the present state, but future developers adding features to SentryStore which use MSentryPrivilege need to be aware that they are responsible for handling special null values. Having said that I am completely fine with pushing this patch now and following up with a different jira.
Thanks for your patience Arun!
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48781
-----------------------------------------------------------
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48781
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85520>
Should these be initialized to __NULL__ instead?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85521>
SentryStore.toNULLCOL ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85522>
SentryStore.toNULLCOL ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85523>
SentryStore.toNULLCOL ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85524>
SentryStore.toNULLCOL ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85525>
SentryStore.toNULLCOL ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85526>
Shall we use fromNULLCOL() here, as __NULL__ is more like reserved word for db purposes?
- Sravya Tirukkovalur
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48870
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85629>
Nit: remove the unused privilegeName in the constructor ?
- Sravya Tirukkovalur
On July 25, 2014, 9:34 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 9:34 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/
-----------------------------------------------------------
(Updated July 25, 2014, 9:34 p.m.)
Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Fixing some rebasing issues...
Repository: sentry
Description
-------
SENTRY-339 :
Remove the need to construct a privilegeName to check for uniqueness.
- Remove the PrivilegeName column
- Remove the constructPrivilegeName() method
- Updated the sql scripts
Diffs (updated)
-----
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/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/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/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/TestSentryStore.java 7637376
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
Diff: https://reviews.apache.org/r/23600/diff/
Testing
-------
Ran all e2e tests and unit tests.
Thanks,
Arun Suresh
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Xiaomeng Huang <xi...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/#review48707
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/23600/#comment85470>
serverName, dbName...are initlized as empty "", so should we handle it as serverName=="" instead of serverName==null ?
- Xiaomeng Huang
On July 24, 2014, 9:41 p.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23600/
> -----------------------------------------------------------
>
> (Updated July 24, 2014, 9:41 p.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-339 :
>
> Remove the need to construct a privilegeName to check for uniqueness.
> - Remove the PrivilegeName column
> - Remove the constructPrivilegeName() method
> - Updated the sql scripts
>
>
> Diffs
> -----
>
> 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/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/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/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/TestSentryStore.java 7637376
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
>
> Diff: https://reviews.apache.org/r/23600/diff/
>
>
> Testing
> -------
>
> Ran all e2e tests and unit tests.
>
>
> Thanks,
>
> Arun Suresh
>
>
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/
-----------------------------------------------------------
(Updated July 24, 2014, 9:41 p.m.)
Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
@Brock, Thanks for the catch.. yep oracle does screw up the empty strings.
I have modified the patch to convert empty/null strings to "__NULL__".
Ideally, I would have liked JDO to handle these Db disparities..
Repository: sentry
Description
-------
SENTRY-339 :
Remove the need to construct a privilegeName to check for uniqueness.
- Remove the PrivilegeName column
- Remove the constructPrivilegeName() method
- Updated the sql scripts
Diffs (updated)
-----
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/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/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/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/TestSentryStore.java 7637376
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
Diff: https://reviews.apache.org/r/23600/diff/
Testing
-------
Ran all e2e tests and unit tests.
Thanks,
Arun Suresh
Re: Review Request 23600: SENTRY-339 : Remove PrivilegeName column and
constructPrivilegeName() function
Posted by Arun Suresh <ar...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23600/
-----------------------------------------------------------
(Updated July 24, 2014, 12:32 a.m.)
Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya Tirukkovalur.
Changes
-------
Updating patch with sravya's comments. Thanks sravya !!
@Sravya,
With regard to the lowercase-ing, I have stuck to explicitly making it lowercase. I guess our agreement was that action will be lowercase rite ? the reason we are seeing some tests failing is that certain testcases set the action as uppercase explicitly and is handled incorrectly downstream.. I am of the opinion that using safeStickLower() would solve the problem.
Repository: sentry
Description
-------
SENTRY-339 :
Remove the need to construct a privilegeName to check for uniqueness.
- Remove the PrivilegeName column
- Remove the constructPrivilegeName() method
- Updated the sql scripts
Diffs (updated)
-----
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/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/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/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/TestSentryStore.java 7637376
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java 79579c6
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java a4ae291
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java 948b0c4
Diff: https://reviews.apache.org/r/23600/diff/
Testing
-------
Ran all e2e tests and unit tests.
Thanks,
Arun Suresh