[GitHub] bigtop pull request #344: BIGTOP-3007: add properties to hive-site.xml for z...
Github user asfgit closed the pull request at: https://github.com/apache/bigtop/pull/344 ---
[GitHub] bigtop pull request #344: BIGTOP-3007: add properties to hive-site.xml for z...
Github user jamesbeedy commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/344#discussion_r172086152
--- Diff:
bigtop-packages/src/charm/hive/layer-hive/lib/charms/layer/bigtop_hive.py ---
@@ -36,31 +36,45 @@ def install(self, hbase=None):
# Prep config
roles = ['hive-client', 'hive-metastore', 'hive-server2']
metastore = "thrift://{}:9083".format(hookenv.unit_private_ip())
-if hbase:
-roles.append('hive-hbase')
-hb_connect = "{}:{}".format(hbase['host'],
hbase['master_port'])
-zk_connect = hbase['zk_connect']
-else:
-hb_connect = ""
-zk_connect = ""
--- End diff --
@kwmonroe totally, nice catch. I'll set those bits appropriately.
---
[GitHub] bigtop pull request #344: BIGTOP-3007: add properties to hive-site.xml for z...
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/344#discussion_r172070816
--- Diff:
bigtop-packages/src/charm/hive/layer-hive/lib/charms/layer/bigtop_hive.py ---
@@ -36,31 +36,45 @@ def install(self, hbase=None):
# Prep config
roles = ['hive-client', 'hive-metastore', 'hive-server2']
metastore = "thrift://{}:9083".format(hookenv.unit_private_ip())
-if hbase:
-roles.append('hive-hbase')
-hb_connect = "{}:{}".format(hbase['host'],
hbase['master_port'])
-zk_connect = hbase['zk_connect']
-else:
-hb_connect = ""
-zk_connect = ""
--- End diff --
Upon closer inspection, there's a side effect of not forcefully setting
this to `""`. With no value, we fall into the default provided by
`cluster.yaml`:
https://github.com/apache/bigtop/blob/master/bigtop-deploy/puppet/hieradata/bigtop/cluster.yaml#L191
which falls up to the hbase config:
https://github.com/apache/bigtop/blob/master/bigtop-deploy/puppet/hieradata/bigtop/cluster.yaml#L132
which makes the namenode (bigtop head node) the default value. This means
on a deployed hive charm, you'll get the following in the config, even when
hbase is not deployed at all:
```
hbase.zookeeper.quorum
ip-172-31-68-129.ec2.internal
http://wiki.apache.org/hadoop/Hive/HBaseIntegration
```
That hostname is for my namenode. This may not be a problem as the hive
unit in this case has no hbase roles defined, but I haven't put "hive plus some
hbase config even when hbase isnt there" to the test to see if anything is
afoul.
---
[GitHub] bigtop pull request #344: BIGTOP-3007: add properties to hive-site.xml for z...
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/344#discussion_r171912690
--- Diff: bigtop-deploy/puppet/hieradata/bigtop/cluster.yaml ---
@@ -192,6 +192,9 @@ hadoop_hive::common_config::hbase_zookeeper_quorum:
"%{hiera('hadoop_hbase::comm
hadoop_hive::common_config::kerberos_realm:
"%{hiera('kerberos::site::realm')}"
hadoop_hive::common_config::metastore_uris:
"thrift://%{hiera('bigtop::hadoop_head_node')}:9083"
+hadoop_hive::common_config::hive_zookeeper_quorum:
"%{hiera('hadoop_hive::common_config::hive_zookeeper_quorum')}"
--- End diff --
Doh! @jamesbeedy you ninja'd me :) Thx for removing `cluster.yaml`
updates.
---
[GitHub] bigtop pull request #344: BIGTOP-3007: add properties to hive-site.xml for z...
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/344#discussion_r171910993
--- Diff: bigtop-deploy/puppet/hieradata/bigtop/cluster.yaml ---
@@ -192,6 +192,9 @@ hadoop_hive::common_config::hbase_zookeeper_quorum:
"%{hiera('hadoop_hbase::comm
hadoop_hive::common_config::kerberos_realm:
"%{hiera('kerberos::site::realm')}"
hadoop_hive::common_config::metastore_uris:
"thrift://%{hiera('bigtop::hadoop_head_node')}:9083"
+hadoop_hive::common_config::hive_zookeeper_quorum:
"%{hiera('hadoop_hive::common_config::hive_zookeeper_quorum')}"
--- End diff --
Per the dev list
[discussion](https://mail-archives.apache.org/mod_mbox/bigtop-dev/201803.mbox/%3C85DF9170-CEC9-4462-B554-5CAF892B0570%40oflebbe.de%3E),
we don't need to alter cluster.yaml for these new properties.
---
[GitHub] bigtop pull request #344: BIGTOP-3007: add properties to hive-site.xml for z...
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/344#discussion_r171911797
--- Diff:
bigtop-packages/src/charm/hive/layer-hive/lib/charms/layer/bigtop_hive.py ---
@@ -69,35 +83,44 @@ def configure_hive(self):
config = hookenv.config()
hive_env = self.dist_config.path('hive_conf') / 'hive-env.sh'
utils.re_edit_in_place(hive_env, {
-r'.*export HADOOP_HEAPSIZE *=.*': 'export HADOOP_HEAPSIZE=%s'
% config['heap'],
--- End diff --
Looks like the rest of the changes to this file are lint fixes. While they
are much appreciated, in the future, please make them in a separate PR. It's a
bit harder to focus on the functional code changes with these formatting
changes.
---
[GitHub] bigtop pull request #344: BIGTOP-3007: add properties to hive-site.xml for z...
Github user kwmonroe commented on a diff in the pull request:
https://github.com/apache/bigtop/pull/344#discussion_r171912318
--- Diff: bigtop-packages/src/charm/hive/layer-hive/reactive/hive.py ---
@@ -78,9 +87,13 @@ def install_hive(hadoop):
else:
hbserver = None
+# Get zookeepers or None
+zks = KV.get('zookeepers', None)
--- End diff --
I'm curious why we're using `unitdata.kv` here. If you look a few lines
higher at the hbase block, we could use the same logic to get zks when
`is_state(zookeeper.ready)` is available.
---
