----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68630/#review208398 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 23 (patched) <https://reviews.apache.org/r/68630/#comment292289> Unused import. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 41 (patched) <https://reviews.apache.org/r/68630/#comment292290> Unused import. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 47 (patched) <https://reviews.apache.org/r/68630/#comment292291> Unused import. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 55 (patched) <https://reviews.apache.org/r/68630/#comment292293> The conf variable is used only in the constructor. No need to keep it in a member variable. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 58 (patched) <https://reviews.apache.org/r/68630/#comment292292> The constructor visibility can be changed to package-private, since it is instantiated through FallbackHiveAuthorizerFactory. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 94 (patched) <https://reviews.apache.org/r/68630/#comment292294> The next few methods are declared to throw HiveAccessControlException, though it is never thrown in the method. Let's try to keep the code as clean and simple as possible. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 148 (patched) <https://reviews.apache.org/r/68630/#comment292297> No need to explicitly declare the type argument. You can use type inference in case of generic instance creation. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 159 (patched) <https://reviews.apache.org/r/68630/#comment292298> Unnecessary throws clause declaration. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 164-171 (patched) <https://reviews.apache.org/r/68630/#comment292300> This section can be replaced with a more elegant implementation, using stream api. ```java if (admins != null && Arrays.stream(admins).parallel().anyMatch(n -> n.equals(userName)) { return; } ``` ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 180-187 (patched) <https://reviews.apache.org/r/68630/#comment292301> I'm not sure this loop is correct, since it's only checking the first element of the hiveObjects list, afterwards breaks the loop. If the intention was to check only the first entry, no need to use a loop. ``` boolean needAdmin = !hiveObjects.isEmpty() && hiveObjects.get(0).getType() == HivePrivilegeObject.HivePrivilegeObjectType.LOCAL_URI; ``` If you want to check all the entries, than this implementation is incorrect. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 188 (patched) <https://reviews.apache.org/r/68630/#comment292302> No need to check for the operation type if needAdmin is already true. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java Lines 203-222 (patched) <https://reviews.apache.org/r/68630/#comment292304> Som unthrown exception declarations. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizerFactory.java Lines 33 (patched) <https://reviews.apache.org/r/68630/#comment292288> The HiveAuthzPluginException is never thrown by the method. Also if you remove the exception the ```import org.apache.hadoop.hive.ql.security.authorization.plugin.HiveAuthzPluginException;``` can be removed. - Laszlo Pinter On Sept. 5, 2018, 5:27 p.m., Daniel Dai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68630/ > ----------------------------------------------------------- > > (Updated Sept. 5, 2018, 5:27 p.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > See HIVE-20420 > > > Diffs > ----- > > ql/pom.xml a55cbe3 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/SettableConfigUpdater.java > 12be41c > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/fallback/FallbackHiveAuthorizerFactory.java > PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_addjar.q PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_compile.q PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_create_func1.q PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_create_func2.q PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_dfs.q PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_disallow_transform.q > PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_load.q PRE-CREATION > ql/src/test/queries/clientnegative/fallbackauth_set_invalidconf.q > PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_addjar.q.out PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_compile.q.out PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_create_func1.q.out > PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_create_func2.q.out > PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_dfs.q.out PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_disallow_transform.q.out > PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_load.q.out PRE-CREATION > ql/src/test/results/clientnegative/fallbackauth_set_invalidconf.q.out > PRE-CREATION > > > Diff: https://reviews.apache.org/r/68630/diff/1/ > > > Testing > ------- > > > Thanks, > > Daniel Dai > >
