hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gabor Szadovszky <gabor.szadovs...@cloudera.com>
Subject Re: Review Request 51593: HIVE-14063 : Add beeline connection configuration file to automatically connect to HS2 using beeline
Date Fri, 02 Sep 2016 21:32:59 GMT

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



Thanks for the patch.
Had some mostly minor findigs. Otherwise LGTM.


beeline/src/java/org/apache/hive/beeline/BeeLine.java (line 913)
<https://reviews.apache.org/r/51593/#comment214891>

    Do we need this one to be public? Seems that is only used inside this class.
    Please, consider narrowing the visibility of the other new methods in this class.



beeline/src/java/org/apache/hive/beeline/hs2connection/HS2ConnectionFileUtils.java (line 99)
<https://reviews.apache.org/r/51593/#comment214899>

    This check seems to be useless as the tokens are split on '=' and the result is properly
checked in the next loop.



beeline/src/java/org/apache/hive/beeline/hs2connection/HiveSiteHS2ConnectionFileParser.java
(line 58)
<https://reviews.apache.org/r/51593/#comment214900>

    I would suggest putting the test classes into the same package so you can tighten the
visibility to package private.



beeline/src/java/org/apache/hive/beeline/hs2connection/UserHS2ConnectionFileParser.java (line
64)
<https://reviews.apache.org/r/51593/#comment214903>

    I would suggest putting the test classes into the same package so you can tighten the
visibility to package private.



beeline/src/java/org/apache/hive/beeline/hs2connection/UserHS2ConnectionFileParser.java (line
78)
<https://reviews.apache.org/r/51593/#comment214904>

    Contrary to the comment it returns an empty Properties object.
    It might worth to comment the return options in the interface.



beeline/src/java/org/apache/hive/beeline/hs2connection/UserHS2ConnectionFileParser.java (line
91)
<https://reviews.apache.org/r/51593/#comment214906>

    I would use foreach and move the try-catch around it. It would be a bit more readable.



itests/hive-unit/src/test/java/org/apache/hive/beeline/hs2connectionfile/TestBeelineWithHS2ConnectionFile.java
(line 190)
<https://reviews.apache.org/r/51593/#comment214913>

    It seems that the last 3 methods have some common code. I would consider moving the common
code parts to one place.



itests/hive-unit/src/test/java/org/apache/hive/beeline/hs2connectionfile/TestBeelineWithHS2ConnectionFile.java
(line 219)
<https://reviews.apache.org/r/51593/#comment214912>

    I guess, it was a debug output only...


- Gabor Szadovszky


On Sept. 2, 2016, 6:30 a.m., Vihang Karajgaonkar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51593/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2016, 6:30 a.m.)
> 
> 
> Review request for hive, Mohit Sabharwal, Sergio Pena, and Szehon Ho.
> 
> 
> Bugs: HIVE-14063
>     https://issues.apache.org/jira/browse/HIVE-14063
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> This change adds a new optional configuration file for Beeline. If this file is present
at predefined locations, Beeline will attempt to create the connection url using the hive-site.xml
found in classpath and another user-specific configuration file. Beeline then connects automatically
using the url generated based on these configuration files. The main objective of the change
is to provide user another way to connect to the HiveServer2 without providing the connection
url everytime. The configuration file uses hadoop xml format so that we can support encryption/obfuscation
using hadoop credential manager API in the future.
> 
> Properties in the user-specific configuration file override the properties derived from
hive-site.xml.
> 
> Tested using newly added unit tests and itests in various Hiveserver2 configurations.
> 
> 
> Diffs
> -----
> 
>   beeline/src/java/org/apache/hive/beeline/BeeLine.java 8e65e3987398531cce5c65c383762cf49a52c578

>   beeline/src/java/org/apache/hive/beeline/Commands.java 2f3ec134098dfa3767bab9545438d1f38f11697c

>   beeline/src/java/org/apache/hive/beeline/hs2connection/BeelineHS2ConnectionFileParseException.java
PRE-CREATION 
>   beeline/src/java/org/apache/hive/beeline/hs2connection/HS2ConnectionFileParser.java
PRE-CREATION 
>   beeline/src/java/org/apache/hive/beeline/hs2connection/HS2ConnectionFileUtils.java
PRE-CREATION 
>   beeline/src/java/org/apache/hive/beeline/hs2connection/HiveSiteHS2ConnectionFileParser.java
PRE-CREATION 
>   beeline/src/java/org/apache/hive/beeline/hs2connection/UserHS2ConnectionFileParser.java
PRE-CREATION 
>   beeline/src/test/org/apache/hive/beeline/TestUserHS2ConnectionFileParser.java PRE-CREATION

>   beeline/src/test/resources/hive-site.xml 5f310d68245275ac9dc24df45579784019eea332 
>   beeline/src/test/resources/test-hs2-conn-conf-kerberos-http.xml PRE-CREATION 
>   beeline/src/test/resources/test-hs2-conn-conf-kerberos-nossl.xml PRE-CREATION 
>   beeline/src/test/resources/test-hs2-conn-conf-kerberos-ssl.xml PRE-CREATION 
>   beeline/src/test/resources/test-hs2-connection-conf-list.xml PRE-CREATION 
>   beeline/src/test/resources/test-hs2-connection-config-noauth.xml PRE-CREATION 
>   beeline/src/test/resources/test-hs2-connection-multi-conf-list.xml PRE-CREATION 
>   beeline/src/test/resources/test-hs2-connection-zookeeper-config.xml PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/beeline/hs2connectionfile/TestBeelineConnectionUsingHiveSite.java
PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/beeline/hs2connectionfile/TestBeelineWithHS2ConnectionFile.java
PRE-CREATION 
>   itests/hive-unit/src/test/java/org/apache/hive/beeline/hs2connectionfile/TestBeelineWithUserHs2ConnectionFile.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51593/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>


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