> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 1624 > > <https://reviews.apache.org/r/54526/diff/2/?file=1596329#file1596329line1624> > > > > Do you know why isMultiActionsSupported is needed? The existing comment > > is not quite clear to me. > > Vamsee Yarlagadda wrote: > Updated the description of the review to address your question. > > Vamsee Yarlagadda wrote: > This method is used to check which entities support multiple actions > (QUERY, UPDATE, ALL). As per the comment, it looks like Database and Table > are the ones that support all three. Do you think even Server, Column support > all actions? If so, we can get rid of the entire method.
I am also not quite sure here. As this code exist quite a while should we change its behavior before we are complete clear? - Hao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54526/#review160558 ----------------------------------------------------------- On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54526/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2017, 8:15 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar > kalvagadda. > > > Repository: sentry > > > Description > ------- > > 1. > isMultiActionsSupported used to return true for all invocations as > tPrivilege.getDbName() is never set to NULL. > So we should change it instead to do a proper check for null - > SentryStore.isNull() instead. > > 2. Currently this method is only dealing database name string but it should > also check for table name strings as mentioned in the comments of the above > method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level > privileges". So fixes the method to handle tables as well. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 3f3afb7080ee330fedd48f4d400553fe14d57deb > > 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 > >
