> On Dec. 8, 2016, 9:32 p.m., Alexander Kolbasov wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1607 > > <https://reviews.apache.org/r/54526/diff/1/?file=1579704#file1579704line1607> > > > > So this is just the rename - right? The point is that getDbName is > > never NULL. Or did you intend to check for the actual __NULL__ string here? > > Vamsee Yarlagadda wrote: > With the change suggested in https://reviews.apache.org/r/54525/, > getDbName could also return a NULL value. So that's why i renamed the > function to check if DB is null or not or we could completely replace the > function call with a conditional statement at the if clauses.
It is with the fix for SENTRY-1541 but not with the current diff. Actually, applying SENTRY-1541 changes the semantics here - suddenly this function is no longer a dummy call. So we either want to preserve the original semantics (and this means removing the function altogether) or changing to new one (in which case we should be confident that it is correct). - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/#review158572 ----------------------------------------------------------- On Dec. 8, 2016, 9:02 a.m., Vamsee Yarlagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54526/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2016, 9:02 a.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > ------- > > Renames the method isMultiActionsSupported to isDBNull to reflect the right > action being performed. > Change based on: https://reviews.apache.org/r/54525/ > > Should we remove the method as the whole and do the DB null check directly at > the if clauses? > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f773a4443e81c5cde3aca0056a2e33d528bf4ec9 > > Diff: https://reviews.apache.org/r/54526/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 > >
