----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72246/#review220000 -----------------------------------------------------------
LGTM, small remarks common/src/java/org/apache/hadoop/hive/conf/HiveConf.java Lines 2675 (patched) <https://reviews.apache.org/r/72246/#comment308247> Keystore/truststore location/password should be added to the "hive.conf.hidden.list". Consider also adding into "hive.conf.restricted.list" common/src/java/org/apache/hadoop/hive/conf/HiveConf.java Line 5622 (original), 5640 (patched) <https://reviews.apache.org/r/72246/#comment308248> Should we consider Builder pattern here? (12 params) hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/ZooKeeperStorage.java Lines 74 (patched) <https://reviews.apache.org/r/72246/#comment308249> Should we keep this comment? hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/ZooKeeperStorage.java Line 69 (original), 83 (patched) <https://reviews.apache.org/r/72246/#comment308258> Could we leverage conf.getZKConfig().getNewZookeeperClient() here? itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/security/TestZooKeeperTokenStore.java Lines 64 (patched) <https://reviews.apache.org/r/72246/#comment308251> Could we start/stop zkCluster only once per test class? itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/security/TestZooKeeperTokenStore.java Lines 117 (patched) <https://reviews.apache.org/r/72246/#comment308259> Should we close the session eventually? itests/hive-unit/src/test/java/org/apache/hive/service/server/TestInformationSchemaWithPrivilege.java Lines 188 (patched) <https://reviews.apache.org/r/72246/#comment308250> Shouldn't we start zookeeperCluster and miniHS2 only once per test class? Do we need shutdown/stop at the end? llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java Line 234 (original), 241 (patched) <https://reviews.apache.org/r/72246/#comment308253> move retryPolicy to the new line ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/CuratorFrameworkSingleton.java Line 54 (original), 51 (patched) <https://reviews.apache.org/r/72246/#comment308254> add 1 more method with empty param list: getNewZookeeperClient() ql/src/test/org/apache/hive/testutils/MiniZooKeeperCluster.java Lines 193 (patched) <https://reviews.apache.org/r/72246/#comment308255> Should we set this config when ssl is not enabled? ql/src/test/org/apache/hive/testutils/MiniZooKeeperCluster.java Line 255 (original), 289 (patched) <https://reviews.apache.org/r/72246/#comment308256> Could you please elaborate on this? - Denys Kuzmenko On March 19, 2020, 9:57 a.m., Peter Varga wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72246/ > ----------------------------------------------------------- > > (Updated March 19, 2020, 9:57 a.m.) > > > Review request for hive, Denys Kuzmenko and Peter Vary. > > > Repository: hive-git > > > Description > ------- > > Enable SSL communication with Zookeeper in Hive > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d50912b4e2 > > hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/ZooKeeperStorage.java > 1fc8d36394 > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/security/TestZooKeeperTokenStore.java > 603155bf8f > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/security/TestZookeeperTokenStoreSSLEnabled.java > PRE-CREATION > > itests/hive-unit/src/test/java/org/apache/hive/service/server/TestInformationSchemaWithPrivilege.java > de2e4937a8 > > itests/hive-unit/src/test/java/org/apache/hive/service/server/TestInformationSchemaWithPriviligeZookeeperSSL.java > PRE-CREATION > jdbc/src/java/org/apache/hive/jdbc/Utils.java e23826eb56 > jdbc/src/java/org/apache/hive/jdbc/ZooKeeperHiveClientHelper.java > 759ba8a5ef > > llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java > d28fd1778c > > ql/src/java/org/apache/hadoop/hive/ql/lockmgr/zookeeper/CuratorFrameworkSingleton.java > fa3a382367 > ql/src/test/org/apache/hive/testutils/MiniZooKeeperCluster.java eec628263a > service/src/java/org/apache/hive/service/server/HiveServer2.java fece82e266 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/SSLZookeeperFactory.java > PRE-CREATION > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/ZooKeeperHiveHelper.java > 99f7c97877 > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > fc6a2fd43a > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/security/MetastoreDelegationTokenManager.java > b35dc7ce4b > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/security/ZooKeeperTokenStore.java > af52fcc5f6 > > > Diff: https://reviews.apache.org/r/72246/diff/1/ > > > Testing > ------- > > > Thanks, > > Peter Varga > >
