----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54525/#review159486 -----------------------------------------------------------
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/#comment230496> 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. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1450) <https://reviews.apache.org/r/54525/#comment230494> Pleaase avoid comparison of booleans in the of statements - it just makes the code more difficult to read. So instead of if (condition == false) a simple if (!condition) works better. 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/#comment230498> I think this is in SENTRY-1540. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1645) <https://reviews.apache.org/r/54525/#comment230499> can you mentioned that this was the original code rather than just leave this commented out? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1721) <https://reviews.apache.org/r/54525/#comment230497> Same comment - can we just copy fields and null fields will automatically become null fields in the result. - Alexander Kolbasov 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 > >
