----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17859/#review33979 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g <https://reviews.apache.org/r/17859/#comment63901> should we use KW_ROLES instead of KW_ROLE ? That will be consistent with 'show roles'. Also, this command can return one or more roles. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveAuthorizer.java <https://reviews.apache.org/r/17859/#comment63855> there is a HiveRole in the interface. We should use the same role representation here. I am OK going either way. I was thinking that using the lightweight classes would be cleaner, as interface implementations would not need to deal with thrift dependencies, but the code to convert between thrift types and these types is all over the place. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java <https://reviews.apache.org/r/17859/#comment63862> It is not safe to cache the metastoreclient object. This can get invalidated between calls to authorization (based on logic in Hive class). So it is safer to cache the factory and get the current valid metastoreclient each time (which uses Hive.get().getMSC()). ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java <https://reviews.apache.org/r/17859/#comment63895> can you add a comment - // reset to default roles if rolename is NULL ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java <https://reviews.apache.org/r/17859/#comment63897> can you add a comment - // set to given role if user belongs to it ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java <https://reviews.apache.org/r/17859/#comment63900> the behavior between setCurrentRole and getCurrentRoles is a little inconsistent. setCurrentRole always gets latest set of roles for user from metastore, but getCurrentRoles gets cached results. I think it is better to always return the latest result and not cache it at all. This api call is not going to be common on frequent. Caching it in constructor means that we would be making this call in all cases, to improve the performance in a small number of cases. - Thejas Nair On Feb. 7, 2014, 11:12 p.m., Ashutosh Chauhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17859/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 11:12 p.m.) > > > Review request for hive. > > > Bugs: HIVE-5930 > https://issues.apache.org/jira/browse/HIVE-5930 > > > Repository: hive-git > > > Description > ------- > > Implements set role and show current role functionality. > > > Diffs > ----- > > cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 06c3f6c > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 32831fa > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java > 9f15609 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 7e69912 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java > 2495c40 > > ql/src/java/org/apache/hadoop/hive/ql/parse/authorization/HiveAuthorizationTaskFactory.java > 1416c2e > > ql/src/java/org/apache/hadoop/hive/ql/parse/authorization/HiveAuthorizationTaskFactoryImpl.java > e91258a > ql/src/java/org/apache/hadoop/hive/ql/plan/RoleDDLDesc.java 77853c5 > > ql/src/java/org/apache/hadoop/hive/ql/processors/CommandProcessorFactory.java > 0ad2fde > ql/src/java/org/apache/hadoop/hive/ql/processors/HiveCommand.java 280d94e > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveAccessController.java > 008efb1 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveAuthorizer.java > 632901e > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveAuthorizerFactory.java > c004105 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HiveAuthorizerImpl.java > 172746e > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java > 5c5d0e5 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizerFactory.java > 7688bbf > > ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java > 732897f > ql/src/test/queries/clientpositive/authorization_set_show_current_role.q > PRE-CREATION > > ql/src/test/results/clientpositive/authorization_set_show_current_role.q.out > PRE-CREATION > > service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java > d00db1c > > Diff: https://reviews.apache.org/r/17859/diff/ > > > Testing > ------- > > New test case is added. > > > Thanks, > > Ashutosh Chauhan > >
