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