----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65498/#review199112 -----------------------------------------------------------
itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestActivePassiveHA.java Lines 123 (patched) <https://reviews.apache.org/r/65498/#comment279405> nit: why is a thread needed when we just join immediately? itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestActivePassiveHA.java Lines 178 (patched) <https://reviews.apache.org/r/65498/#comment279406> nit: could it wait or loop on some condition for 3sec instead of sleeping? e.g. hs2Instances.size or leaders.size below llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java Line 330 (original), 330 (patched) <https://reviews.apache.org/r/65498/#comment279407> hmm... why does getInstances now get one instance? llap-client/src/java/org/apache/hadoop/hive/registry/impl/ServiceInstanceBase.java Lines 67 (patched) <https://reviews.apache.org/r/65498/#comment279409> why is this change necessary? shouldn't worker ID be enough? llap-client/src/java/org/apache/hadoop/hive/registry/impl/ServiceInstanceBase.java Line 65 (original), 73 (patched) <https://reviews.apache.org/r/65498/#comment279408> nit: this is missing another 31 llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java Lines 84 (patched) <https://reviews.apache.org/r/65498/#comment279410> hmm... if these are moved here, should they still be parameters to ctor? I saw GROUP being added to all the ctors calling super(), but given that they are taken from here might as well just hide them inside here llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java Line 86 (original), 91 (patched) <https://reviews.apache.org/r/65498/#comment279411> comment is out of date llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java Lines 168 (patched) <https://reviews.apache.org/r/65498/#comment279412> hmm.. I'm not sure what this change is about. ACL provider may be called for multiple paths that do not exist; some may be top level paths that should have full access, and some should be user level paths that should be restricted. Generally there's a clear boundary - everything above certain dir is open, everything at or below is restricted, so only one path argument is necessary. Why is this change needed? llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java Line 406 (original), 459 (patched) <https://reviews.apache.org/r/65498/#comment279413> why not final? llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java Lines 482 (patched) <https://reviews.apache.org/r/65498/#comment279414> should null be logged? not sure when it would be valid for correct data to have null here, if there's no bug of some sort llap-common/src/java/org/apache/hadoop/hive/llap/metrics/MetricsUtils.java Lines 27 (patched) <https://reviews.apache.org/r/65498/#comment279415> hmm llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskSchedulerService.java Line 1365 (original), 1375 (patched) <https://reviews.apache.org/r/65498/#comment279416> hmm... this cast doesn't look so good. Why was field type changed? Maybe the cast should be at assignment - Sergey Shelukhin On March 13, 2018, 6:41 p.m., Prasanth_J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65498/ > ----------------------------------------------------------- > > (Updated March 13, 2018, 6:41 p.m.) > > > Review request for hive, Sergey Shelukhin and Thejas Nair. > > > Bugs: HIVE-18281 > https://issues.apache.org/jira/browse/HIVE-18281 > > > Repository: hive-git > > > Description > ------- > > HIVE-18281: HiveServer2 HA for LLAP and Workload Manager > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java aedd1ec > > itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestActivePassiveHA.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/AbstractHiveService.java > 6cab8cd > itests/util/src/main/java/org/apache/hive/jdbc/miniHS2/MiniHS2.java 8bbf8a4 > > llap-client/src/java/org/apache/hadoop/hive/llap/registry/ServiceRegistry.java > 5d7f813 > > llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapFixedRegistryImpl.java > c88198f > > llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapRegistryService.java > 80a6aba > > llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java > 8339230 > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClient.java > 32d5caa > llap-client/src/java/org/apache/hadoop/hive/registry/RegistryUtilities.java > PRE-CREATION > llap-client/src/java/org/apache/hadoop/hive/registry/ServiceInstance.java > 908b3bb > > llap-client/src/java/org/apache/hadoop/hive/registry/ServiceInstanceSet.java > 34fba5c > > llap-client/src/java/org/apache/hadoop/hive/registry/impl/ServiceInstanceBase.java > db3d788 > > llap-client/src/java/org/apache/hadoop/hive/registry/impl/TezAmInstance.java > 0724cf5 > > llap-client/src/java/org/apache/hadoop/hive/registry/impl/TezAmRegistryImpl.java > 417e571 > > llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java > 17269dd > llap-common/src/java/org/apache/hadoop/hive/llap/metrics/MetricsUtils.java > 9666517 > > llap-ext-client/src/java/org/apache/hadoop/hive/llap/LlapBaseInputFormat.java > 0120639 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/services/impl/LlapWebServices.java > 58bf8dc > > llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskSchedulerService.java > 66de3b8 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionPoolManager.java > 46cfe56 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java b98fb58 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/Utils.java bc438bb > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/LlapClusterStateForCompile.java > a8d729d > ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestTezSessionPool.java > d261623 > > service/src/java/org/apache/hive/service/server/HS2ActivePassiveHARegistry.java > PRE-CREATION > > service/src/java/org/apache/hive/service/server/HS2ActivePassiveHARegistryClient.java > PRE-CREATION > service/src/java/org/apache/hive/service/server/HiveServer2.java b7ece2b > > service/src/java/org/apache/hive/service/server/HiveServer2HAInstanceSet.java > PRE-CREATION > service/src/java/org/apache/hive/service/server/HiveServer2Instance.java > PRE-CREATION > service/src/java/org/apache/hive/service/servlet/HS2LeadershipStatus.java > PRE-CREATION > service/src/java/org/apache/hive/service/servlet/HS2Peers.java PRE-CREATION > > > Diff: https://reviews.apache.org/r/65498/diff/3/ > > > Testing > ------- > > > Thanks, > > Prasanth_J > >
