----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54464/#review158589 -----------------------------------------------------------
The fix is good, a few suggestions for the test. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java (line 384) <https://reviews.apache.org/r/54464/#comment229368> Can you split this into a separate test for groups and users? This will simplify merge with CDH which has groups but not users. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java (line 393) <https://reviews.apache.org/r/54464/#comment229370> It would be better to write a test in a way that would allow a set of roles as input rather then a few fixed values. Then we can construct a test e.g. with 0, 1 and N values. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java (line 405) <https://reviews.apache.org/r/54464/#comment229366> use createRole() instead here and below. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java (line 419) <https://reviews.apache.org/r/54464/#comment229369> I think a more accurate test verifies that each roleName is in tRoles and each item in tRoles is in the {roleName{1,2,3}}set. In practice your test does test for this condition, but it is not generalizable for a set of roles. - Alexander Kolbasov On Dec. 8, 2016, 7:47 a.m., Vamsee Yarlagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54464/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2016, 7:47 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > ------- > > This change will ensure that multiple calls to the DB are replaced with a > single call that looks for multiple matching values. > > More reference: > Checkout "Collections" section under > http://etutorials.org/Programming/Java+data+objects/Chapter+9.+The+JDO+Query+Language/9.6+The+Query+Filter/ > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 64df6a5655cf2c121cb44f2274369fbe9d70ec83 > > Diff: https://reviews.apache.org/r/54464/diff/ > > > Testing > ------- > > Verified the changes by running the unit tests of TestSentryStore and all 40 > tests passed. > > ```bash > vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore > ... > ------------------------------------------------------- > T E S T S > ------------------------------------------------------- > Running org.apache.sentry.provider.db.service.persistent.TestSentryStore > 2016-12-07 23:46:42.147 java[5054:534219] Unable to load realm info from > SCDynamicStore > Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.538 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: 42.003s > [INFO] Finished at: Wed Dec 07 23:47:00 PST 2016 > [INFO] Final Memory: 59M/660M > [INFO] > ------------------------------------------------------------------------ > ``` > > > Thanks, > > Vamsee Yarlagadda > >
