hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thejas Nair" <the...@hortonworks.com>
Subject Re: Review Request 17859: Implements set role and show current role functionality.
Date Sat, 08 Feb 2014 01:16:03 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message