You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Ashutosh Chauhan <ha...@apache.org> on 2014/02/07 23:22:26 UTC

Review Request 17859: Implements set role and show current role functionality.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17859/
-----------------------------------------------------------

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

Re: Review Request 17859: Implements set role and show current role functionality.

Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17859/#review33971
-----------------------------------------------------------



ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java
<https://reviews.apache.org/r/17859/#comment63826>

    It's possible the exception doesn't have a cause and therefore we are eating the true exception.
    
    This should be throw new ...("msg here", e);



ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
<https://reviews.apache.org/r/17859/#comment63827>

    Why not add a new test for set role?


- Brock Noland


On Feb. 7, 2014, 10:22 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, 10:22 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
> 
>

Re: Review Request 17859: Implements set role and show current role functionality.

Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 17859: Implements set role and show current role functionality.

Posted by Thejas Nair <th...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17859/#review34008
-----------------------------------------------------------

Ship it!


Ship It!

- Thejas Nair


On Feb. 8, 2014, 2:39 a.m., Ashutosh Chauhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17859/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2014, 2:39 a.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
> 
>

Re: Review Request 17859: Implements set role and show current role functionality.

Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17859/
-----------------------------------------------------------

(Updated Feb. 8, 2014, 2:39 a.m.)


Review request for hive.


Changes
-------

Incorporated Thejas comments.


Bugs: HIVE-5930
    https://issues.apache.org/jira/browse/HIVE-5930


Repository: hive-git


Description
-------

Implements set role and show current role functionality.


Diffs (updated)
-----

  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

Re: Review Request 17859: Implements set role and show current role functionality.

Posted by Ashutosh Chauhan <ha...@apache.org>.
-----------------------------------------------------------
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.


Changes
-------

Addressed Brock's comments.


Bugs: HIVE-5930
    https://issues.apache.org/jira/browse/HIVE-5930


Repository: hive-git


Description
-------

Implements set role and show current role functionality.


Diffs (updated)
-----

  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