hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thejas Nair" <the...@hortonworks.com>
Subject Re: Review Request 37660: HiveServer2 should store connection params in ZK when using dynamic service discovery for simpler client connection string.
Date Fri, 21 Aug 2015 22:09:08 GMT

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



jdbc/src/java/org/apache/hive/jdbc/ZooKeeperHiveClientHelper.java (line 97)
<https://reviews.apache.org/r/37660/#comment151301>

    (this is mostly a nit, as this perf impact would be hard to notice. but the tweaks are
also minor) - 
    
    The pattern can be a static final member. (It is not mutable and thread safe).
    
    I think a slightly more efficient pattern would be -(add '=' to the non key char, that
way the pattern evaluation would not have to back track after going beyond '=').
    
    "([^=;]*)=([^;]*)[;]?"
    
    (seems faster by around 20% in a quick test i did)
    public class TClass {
    
      private static String key;
      private static String value;
    
      public static void main(String[] args) throws InterruptedException {
        long ts = System.currentTimeMillis();
    
        String confStr = "hiveserver.configparam.key1=value1;hiveserver.configparam.key2=value2;hiveserver.configparam.key3=value3;";
        Pattern pattern = Pattern.compile("([^;]*)=([^;]*)[;]?");
    //    for (int i = 0; i < 1000000; i++) {
        for (int i = 0; i < 1; i++) {
          Matcher matcher = pattern.matcher(confStr);
    
          while (matcher.find()) {
            key = matcher.group(1);
            value = matcher.group(2);
    //        System.out.println(key + ":" + value);
          }
        }
    
        System.out.println(System.currentTimeMillis() - ts);
    
      }
    
    }



jdbc/src/java/org/apache/hive/jdbc/ZooKeeperHiveClientHelper.java (line 102)
<https://reviews.apache.org/r/37660/#comment151309>

    The "matcher.group(1) != null" can be moved outside of each if statement to a common one
on top.
    
    if(matcher.group(1) == null) {
     continue;
    }
    If the key is not null but value is null, i think we should throw an error. It would indicate
a bug or some corruption.
    
    if(matcher.group(2) == null) {
     throw ..
    }



jdbc/src/java/org/apache/hive/jdbc/ZooKeeperHiveClientHelper.java (line 103)
<https://reviews.apache.org/r/37660/#comment151312>

    hive configuration is case sensitive, lets keep it consistent here.



service/src/java/org/apache/hive/service/server/HiveServer2.java (line 109)
<https://reviews.apache.org/r/37660/#comment151313>

    typo - "initialize"



service/src/java/org/apache/hive/service/server/HiveServer2.java (line 133)
<https://reviews.apache.org/r/37660/#comment151315>

    we don't seem to have any existing support for HIVE_SERVER2_AUTHENTICATION env variable.



service/src/java/org/apache/hive/service/server/HiveServer2.java (line 220)
<https://reviews.apache.org/r/37660/#comment151316>

    Guava Joiner can be used here (it likely uses StringBuilder underneath) -
    Joiner.on(';').withKeyValueSeparator("=").join(confsToPublish);



service/src/java/org/apache/hive/service/server/HiveServer2.java (line 273)
<https://reviews.apache.org/r/37660/#comment151318>

    do we need to publish the password based authentication modes ?


- Thejas Nair


On Aug. 20, 2015, 10:04 p.m., Vaibhav Gumashta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37660/
> -----------------------------------------------------------
> 
> (Updated Aug. 20, 2015, 10:04 p.m.)
> 
> 
> Review request for hive and Thejas Nair.
> 
> 
> Bugs: HIVE-11581
>     https://issues.apache.org/jira/browse/HIVE-11581
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> https://issues.apache.org/jira/browse/HIVE-11581
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9a6781b 
>   jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java bb2b695 
>   jdbc/src/java/org/apache/hive/jdbc/Utils.java 0e4693b 
>   jdbc/src/java/org/apache/hive/jdbc/ZooKeeperHiveClientHelper.java e24b3dc 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 4a4be97 
> 
> Diff: https://reviews.apache.org/r/37660/diff/
> 
> 
> Testing
> -------
> 
> Manually, will publish test matrix shortly.
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>


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