> On Dec. 16, 2016, 8:18 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1449 > > <https://reviews.apache.org/r/54525/diff/3/?file=1587905#file1587905line1449> > > > > I'd suggest a simpler approach. > > > > For all these fields just do > > > > Privilege.setFoo(mSentryPrivilege.getFoo()) > > > > So all fields which are null in mSentryPrivilege will become null in > > TSentryPrivilege which is exactly what we want without any extra ifs.
It looks like when we are trying to save a null string to the database (JDO) we seem to be passing the string "__NULL__" instead. So it is kinda of essential to do the check if the string is needed null by doing isNULL before setting to TSentryPrivilege. Here is where we are converting to MSentryPrivilege: https://github.com/apache/sentry/blob/55d2721c866f0820f03162d92b0a62768eea2d1b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java#L1472 And toNULLCol will replace all null strings with "__NULL__" string https://github.com/apache/sentry/blob/55d2721c866f0820f03162d92b0a62768eea2d1b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java#L1720 > On Dec. 16, 2016, 8:18 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1644 > > <https://reviews.apache.org/r/54525/diff/3/?file=1587905#file1587905line1644> > > > > I think this is in SENTRY-1540. Reworded the comment to mean that as part of SENTRY-1541, we wanted to ensure to preserve the original behaviour. - Vamsee ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/#review159486 ----------------------------------------------------------- On Dec. 16, 2016, 7:51 p.m., Vamsee Yarlagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54525/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2016, 7:51 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > ------- > > Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 868e67720196f443cd281ec4e80ad552bf86e569 > > Diff: https://reviews.apache.org/r/54525/diff/ > > > Testing > ------- > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test > -Dtest=TestSentryStore -DfailIfNoTests=false > ... > ------------------------------------------------------- > T E S T S > ------------------------------------------------------- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - > in org.apache.sentry.provider.db.service.persistent.TestSentryStore > > Results : > > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 44.421s > [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016 > [INFO] Final Memory: 67M/687M > [INFO] > ------------------------------------------------------------------------ > ``` > > > Thanks, > > Vamsee Yarlagadda > >
