----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66185/#review199704 -----------------------------------------------------------
beeline/src/java/org/apache/hive/beeline/BeeLine.java Lines 321 (patched) <https://reviews.apache.org/r/66185/#comment280113> following pattern of other options "named JDBC URL" might be better here. beeline/src/java/org/apache/hive/beeline/BeeLine.java Lines 322 (patched) <https://reviews.apache.org/r/66185/#comment280115> A comma after "the named JDBC URL to connect to" would make it easier to read. Also, maybe start with upper case "The" ? It looks awkward starting with lower case. Maybe we should change the other ones as well in this/another patch. beeline/src/java/org/apache/hive/beeline/hs2connection/HS2ConnectionFileParser.java Lines 51 (patched) <https://reviews.apache.org/r/66185/#comment280183> I was thinking it would be useful to have 'named' url params, and a different setting for mapping default to a name. So that users can easily modify and put that file in a location they prefer, but still easily understand their current setting. For example, the config would have - <beeline.hs2.jdbc.url.HS2_TEZ>jdbc:/......</..> <beeline.hs2.jdbc.url.HS2_LLAP>jdbc:/......</..> <beeline.hs2.jdbc.url.default.name>HS2_LLAP</..> The user can change default between HS2_TEZ and HS2_LLAP without having to lose the descriptive name. beeline/src/java/org/apache/hive/beeline/hs2connection/HS2ConnectionFileUtils.java Lines 130 (patched) <https://reviews.apache.org/r/66185/#comment280180> It would be useful to print the file name along with error message. Maybe, instead of looking up all urls, you can pass in the url name (if any) to the method in UserHS2ConnectionFileParser ? That way all the entries in config don't need to be traversed, you can just construct the expected config name and look for that. beeline/src/java/org/apache/hive/beeline/hs2connection/UserHS2ConnectionFileParser.java Lines 127 (patched) <https://reviews.apache.org/r/66185/#comment280177> You can use Configuration.getPropsWithPrefix (since we are using Configuration anyway!) beeline/src/test/org/apache/hive/beeline/hs2connection/TestUserHS2ConnectionFileParser.java Line 58 (original), 58 (patched) <https://reviews.apache.org/r/66185/#comment280184> A nit - Adding following method would keep this simpler/easier to read - getParsedUrlFromConfigFile(String filename) { return getParsedUrlFromConfigFile(filename, null); } Or perhaps another method for the named url case getNamedUrlFromConfigFile - Thejas Nair On March 20, 2018, 10:54 p.m., Vaibhav Gumashta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66185/ > ----------------------------------------------------------- > > (Updated March 20, 2018, 10:54 p.m.) > > > Review request for hive, Thejas Nair and Vihang Karajgaonkar. > > > Bugs: HIVE-18963 > https://issues.apache.org/jira/browse/HIVE-18963 > > > Repository: hive-git > > > Description > ------- > > https://issues.apache.org/jira/browse/HIVE-18963 > > > Diffs > ----- > > beeline/src/java/org/apache/hive/beeline/BeeLine.java 402fadddde > > beeline/src/java/org/apache/hive/beeline/hs2connection/HS2ConnectionFileParser.java > b769e8581f > > beeline/src/java/org/apache/hive/beeline/hs2connection/HS2ConnectionFileUtils.java > f635b40633 > > beeline/src/java/org/apache/hive/beeline/hs2connection/UserHS2ConnectionFileParser.java > 2801ebee09 > beeline/src/main/resources/BeeLine.properties 6fca953836 > > beeline/src/test/org/apache/hive/beeline/hs2connection/TestUserHS2ConnectionFileParser.java > 1d17887417 > beeline/src/test/resources/test-hs2-named-connection-config.xml > PRE-CREATION > > itests/hive-unit/src/test/java/org/apache/hive/beeline/hs2connection/BeelineWithHS2ConnectionFileTestBase.java > 3da31ad8a9 > > > Diff: https://reviews.apache.org/r/66185/diff/1/ > > > Testing > ------- > > > Thanks, > > Vaibhav Gumashta > >
