You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Carl Steinbach <ca...@cloudera.com> on 2013/05/09 09:38:19 UTC
Re: Review Request: HIVE-4356 - remove duplicate impersonation parameters
for hiveserver2
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10554/#review20369
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/10554/#comment41943>
Why was hive.server2.enable.doAs chosen over hive.server2.enable.impersonation? Also, this behavior was disabled by default in both CDH4 and HDP 1.2. I'm not sure that flipping the switch now is a great idea, and long term (i.e. with fine-grained authorization) this is definitely the wrong default behavior.
service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
<https://reviews.apache.org/r/10554/#comment41944>
I think this is a lot less clear now that the username code has been pulled out into a separate method. The name "getUserName" implies that it's extracting this information from TOpenSessionReq when that's not actually the case. Please move this back into OpenSession() and add a comment there if you think it's necessary.
service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
<https://reviews.apache.org/r/10554/#comment41945>
Is refactoring this code into a separate protected method really necessary? Every other method in this class is self-contained. I'd like to see that pattern maintained.
service/src/test/org/apache/hive/service/auth/TestPlainSaslHelper.java
<https://reviews.apache.org/r/10554/#comment41946>
Formatting.
service/src/test/org/apache/hive/service/cli/thrift/TestThriftCLIService.java
<https://reviews.apache.org/r/10554/#comment41947>
Formatting
service/src/test/org/apache/hive/service/cli/thrift/TestThriftCLIService.java
<https://reviews.apache.org/r/10554/#comment41950>
Formatting
service/src/test/org/apache/hive/service/cli/thrift/TestThriftCLIService.java
<https://reviews.apache.org/r/10554/#comment41948>
Formatting
service/src/test/org/apache/hive/service/cli/thrift/TestThriftCLIService.java
<https://reviews.apache.org/r/10554/#comment41949>
Formatting
- Carl Steinbach
On April 16, 2013, 9:46 p.m., Thejas Nair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10554/
> -----------------------------------------------------------
>
> (Updated April 16, 2013, 9:46 p.m.)
>
>
> Review request for hive.
>
>
> Description
> -------
>
> remove duplicate impersonation parameters for hiveserver2
>
>
> This addresses bug HIVE-4356.
> https://issues.apache.org/jira/browse/HIVE-4356
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 78d9cc9
> conf/hive-default.xml.template e266ce7
> service/src/java/org/apache/hive/service/auth/PlainSaslHelper.java 18d4aae
> service/src/java/org/apache/hive/service/cli/CLIService.java b53599b
> service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 43d79aa
> service/src/test/org/apache/hive/service/auth/TestPlainSaslHelper.java PRE-CREATION
> service/src/test/org/apache/hive/service/cli/thrift/TestThriftCLIService.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/10554/diff/
>
>
> Testing
> -------
>
> Unit tests included.
> Manually tested on (kerberos) secure and unsecure cluster.
>
>
> Thanks,
>
> Thejas Nair
>
>