> On 四月 7, 2016, 6:23 a.m., Jerry Chen wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java, > > line 179 > > <https://reviews.apache.org/r/34087/diff/3/?file=1327670#file1327670line179> > > > > Suggest to remove: > > Set<String> result = Sets.newHashSet(); > > > > And use the following instead: > > Set<String> result = policy...
Thank you for catching it. > On 四月 7, 2016, 6:23 a.m., Jerry Chen wrote: > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java, > > line 99 > > <https://reviews.apache.org/r/34087/diff/3/?file=1327672#file1327672line99> > > > > Question: What is appendUserGroupsMappings for? Do we need this? Is it > > used only for test cases? > > > > The naming may need to be reconsidered if we want to keep it. Names > > like UserGroupsMappings is very tricky. Consider a simple name: > > addUserGroupMappings? Yes, we need, currently setUserGroupsMappings() can only accpect `HashMap`, it's one-one mapping, the UserGroupMapping in PolicyFile is Multimap, it's one-many mapping, the old test case didn't cover one-many user group mapping, adding this method would be the a better solution than calling setUserGroupsMappings several times. I will rename it to `addUserGroupMappings`, Thanks. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34087/#review127561 ----------------------------------------------------------- On 四月 6, 2016, 2:58 p.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34087/ > ----------------------------------------------------------- > > (Updated 四月 6, 2016, 2:58 p.m.) > > > Review request for sentry, Dapeng Sun and Jerry Chen. > > > Repository: sentry > > > Description > ------- > > Update AuthorizationProvider and e2e test for grant user to role > > > Diffs > ----- > > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > 0cf0b5d > > sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java > 14af2d4 > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/PolicyFile.java > 991a95f > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 8515a2b > > Diff: https://reviews.apache.org/r/34087/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
