> 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
>
>
