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