----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64452/#review196435 -----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java Lines 805-814 (patched) <https://reviews.apache.org/r/64452/#comment276077> This class is not used anymore and will be removed in the future, I don't think we should continue maintaining it. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java Line 550 (original) <https://reviews.apache.org/r/64452/#comment276076> All the HiveAuthzBindingHookBase changes are not related to the fix of this issue. Can we remove those changes to keep patch smaller and the history clean? I think We should fix those issues on other patches that address syntax issues. sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java Lines 107-109 (original), 107-109 (patched) <https://reviews.apache.org/r/64452/#comment276078> What's the reason of chaning to newHashSet() instead of leaving the emptySet()? Both will have empty sets, don't they? Also, is it necessary to change the variable name 'e' to 'ex'? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java Lines 188 (patched) <https://reviews.apache.org/r/64452/#comment276079> This could have a tabl or empty space, Please remove this change. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java Lines 288-291 (patched) <https://reviews.apache.org/r/64452/#comment276083> Can this test be moved to the previous test? I think both tests are similar. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java Line 369 (original), 369 (patched) <https://reviews.apache.org/r/64452/#comment276081> Why do we need the table name change? Was the older name causing problems? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java Lines 371 (patched) <https://reviews.apache.org/r/64452/#comment276080> This could have a tabl or empty space, Please remove this change. - Sergio Pena On Jan. 22, 2018, 9:16 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64452/ > ----------------------------------------------------------- > > (Updated Jan. 22, 2018, 9:16 p.m.) > > > Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio > Pena, and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > Update the code to make sure user without group but with role has proper > access. > test case is added for the scenario that user has no group but with desired > role. > test case is added for the scenario that user has no group and no privilege > to allow the access > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java > 5a21dd3 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java > 447deaf > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > a9b98f3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > edea5b6 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java > 5364937 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestUserManagement.java > fd8ec56 > > > Diff: https://reviews.apache.org/r/64452/diff/3/ > > > Testing > ------- > > unit tests > > > Thanks, > > Na Li > >
