You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/01/12 07:42:59 UTC
[GitHub] [iceberg] dixingxing0 opened a new pull request #2075: Avoid reset hive configuration to default value.
dixingxing0 opened a new pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075
Fixes https://github.com/apache/iceberg/issues/2070
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r558317434
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveConf.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * For https://github.com/apache/iceberg/issues/2070
+ */
+public class TestHiveConf {
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void test() {
+ HiveConf hiveConf = createHiveConf();
Review comment:
There are some issues with how we handle pools in general. There is some work fixing this on another PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary merged pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary merged pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r558321029
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestHiveClientPool {
+
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void testConf() {
+ HiveConf conf = createHiveConf();
+ conf.set(HiveConf.ConfVars.METASTOREURIS.varname, "thrift://locahost:12345");
+ conf.set(HiveConf.ConfVars.METASTOREWAREHOUSE.varname, "file:/mywarehouse/");
+ conf.setInt("iceberg.hive.client-pool-size", 10);
+
+ HiveClientPool clientPool = new HiveClientPool(conf);
+ HiveConf clientConf = clientPool.hiveConf();
+
+ // hive will pick up system property whenever a new HiveConf is created
+ // See org.apache.iceberg.hive.TestHiveMetastore.start(int)
+ if (System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname) != null) {
+ Assert.assertEquals(System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ } else {
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ }
+
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname));
+ Assert.assertEquals(conf.get("iceberg.hive.client-pool-size"), clientConf.get("iceberg.hive.client-pool-size"));
+ Assert.assertEquals(10, clientPool.poolSize());
+
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL.varname),
+ clientConf.get(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL.varname));
+ Assert.assertTrue(clientConf.getBoolVar(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL));
+ }
+
+ @Test
Review comment:
I still do not think this is necessary.
If we want to make sure that we test with a good conf then we should just check the default in the `testConf()` method so we can be sure that the change is done.
What do you think?
Thanks,
Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#issuecomment-796640343
CC: @lcspinter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r558341695
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveConf.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * For https://github.com/apache/iceberg/issues/2070
+ */
+public class TestHiveConf {
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void test() {
+ HiveConf hiveConf = createHiveConf();
Review comment:
> There are some issues with how we handle pools in general. There is some work fixing this on another PR
Glad to hear that!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r558340207
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestHiveClientPool {
+
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void testConf() {
+ HiveConf conf = createHiveConf();
+ conf.set(HiveConf.ConfVars.METASTOREURIS.varname, "thrift://locahost:12345");
+ conf.set(HiveConf.ConfVars.METASTOREWAREHOUSE.varname, "file:/mywarehouse/");
+ conf.setInt("iceberg.hive.client-pool-size", 10);
+
+ HiveClientPool clientPool = new HiveClientPool(conf);
+ HiveConf clientConf = clientPool.hiveConf();
+
+ // hive will pick up system property whenever a new HiveConf is created
+ // See org.apache.iceberg.hive.TestHiveMetastore.start(int)
+ if (System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname) != null) {
+ Assert.assertEquals(System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ } else {
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ }
+
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname));
+ Assert.assertEquals(conf.get("iceberg.hive.client-pool-size"), clientConf.get("iceberg.hive.client-pool-size"));
+ Assert.assertEquals(10, clientPool.poolSize());
+
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL.varname),
+ clientConf.get(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL.varname));
+ Assert.assertTrue(clientConf.getBoolVar(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL));
+ }
+
+ @Test
Review comment:
OK, I just deleted this test case :blush:.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592866290
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestHiveClientPool {
+
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void testConf() {
+ HiveConf conf = createHiveConf();
+ conf.set(HiveConf.ConfVars.METASTOREURIS.varname, "thrift://locahost:12345");
+ conf.set(HiveConf.ConfVars.METASTOREWAREHOUSE.varname, "file:/mywarehouse/");
+ conf.setInt("iceberg.hive.client-pool-size", 10);
+
+ HiveClientPool clientPool = new HiveClientPool(conf);
+ HiveConf clientConf = clientPool.hiveConf();
+
+ // hive will pick up system property whenever a new HiveConf is created
+ // see org.apache.iceberg.hive.TestHiveMetastore.start(int)
+ if (System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname) != null) {
+ Assert.assertEquals(System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ } else {
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ }
Review comment:
Thanks peter, this test branch lead misunderstanding, it's totaly has nothing to do with this PR, i will delete it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592949567
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -45,6 +46,7 @@
public HiveClientPool(int poolSize, Configuration conf) {
super(poolSize, TTransportException.class);
this.hiveConf = new HiveConf(conf, HiveClientPool.class);
+ this.hiveConf.addResource(conf);
Review comment:
> new HiveConf(conf, HiveClientPool.class) will copy all the overlay properties in conf and new HiveConf(HiveClientPool.class) will not.
I agree with your assessment above. My guess is that `this.hiveConf.addResource(conf);` will add the overlay properties too (might be wrong, just asking). If this statement is right we might not need to add it twice.
Thanks, Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592237740
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestHiveClientPool {
+
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void testConf() {
+ HiveConf conf = createHiveConf();
+ conf.set(HiveConf.ConfVars.METASTOREURIS.varname, "thrift://locahost:12345");
+ conf.set(HiveConf.ConfVars.METASTOREWAREHOUSE.varname, "file:/mywarehouse/");
+ conf.setInt("iceberg.hive.client-pool-size", 10);
+
+ HiveClientPool clientPool = new HiveClientPool(conf);
+ HiveConf clientConf = clientPool.hiveConf();
+
+ // hive will pick up system property whenever a new HiveConf is created
+ // see org.apache.iceberg.hive.TestHiveMetastore.start(int)
+ if (System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname) != null) {
+ Assert.assertEquals(System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ } else {
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ }
Review comment:
Do we want to test this case specifically, or it is just happens that this System property is set in some environments and want to avoid this situation. If this is the later, we might just use `System.clearProperty(HiveConf.ConfVars.METASTOREURIS.varname)` to make sure that this not interferes with our tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#issuecomment-797210124
@pvary comments addressed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592955587
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -45,6 +46,7 @@
public HiveClientPool(int poolSize, Configuration conf) {
super(poolSize, TTransportException.class);
this.hiveConf = new HiveConf(conf, HiveClientPool.class);
+ this.hiveConf.addResource(conf);
Review comment:
As i tested, `this.hiveConf.addResource(conf)` will not copy the overlay properties, the test can still pass, but `this.hiveConf.overlay` will not contains the origin overlay properties in `conf`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#issuecomment-801683748
> @rdblue I see you've start marking issues in the 0.11.1 milestone, i think maybe this issue should be added.
As this is a fix with a small risk, added it it to the 0.11.1 milestone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r558340207
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveClientPool.java
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestHiveClientPool {
+
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void testConf() {
+ HiveConf conf = createHiveConf();
+ conf.set(HiveConf.ConfVars.METASTOREURIS.varname, "thrift://locahost:12345");
+ conf.set(HiveConf.ConfVars.METASTOREWAREHOUSE.varname, "file:/mywarehouse/");
+ conf.setInt("iceberg.hive.client-pool-size", 10);
+
+ HiveClientPool clientPool = new HiveClientPool(conf);
+ HiveConf clientConf = clientPool.hiveConf();
+
+ // hive will pick up system property whenever a new HiveConf is created
+ // See org.apache.iceberg.hive.TestHiveMetastore.start(int)
+ if (System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname) != null) {
+ Assert.assertEquals(System.getProperty(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ } else {
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREURIS.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREURIS.varname));
+ }
+
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname),
+ clientConf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname));
+ Assert.assertEquals(conf.get("iceberg.hive.client-pool-size"), clientConf.get("iceberg.hive.client-pool-size"));
+ Assert.assertEquals(10, clientPool.poolSize());
+
+ Assert.assertEquals(conf.get(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL.varname),
+ clientConf.get(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL.varname));
+ Assert.assertTrue(clientConf.getBoolVar(HiveConf.ConfVars.METASTORE_USE_THRIFT_SASL));
+ }
+
+ @Test
Review comment:
OK, I'll remove this test case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592955587
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -45,6 +46,7 @@
public HiveClientPool(int poolSize, Configuration conf) {
super(poolSize, TTransportException.class);
this.hiveConf = new HiveConf(conf, HiveClientPool.class);
+ this.hiveConf.addResource(conf);
Review comment:
As i tested, `this.hiveConf.addResource(conf)` will not copy the overlay properties, so the test can still pass, but `this.hiveConf.overlay` will not contains the origin overlay properties in `conf`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592240739
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -45,6 +46,7 @@
public HiveClientPool(int poolSize, Configuration conf) {
super(poolSize, TTransportException.class);
this.hiveConf = new HiveConf(conf, HiveClientPool.class);
+ this.hiveConf.addResource(conf);
Review comment:
It is strange that we have to provide the `conf` twice in this case.
Do I understand correctly that this is because of the precedence? So in the constructor we set the `conf` properties, then initialize based on the class, but we would like to have the `conf` values higher priority than ones stored in the jars?
Would this work?
```
this.hiveConf = new HiveConf(HiveClientPool.class);
this.hiveConf.addResource(conf);
```
Sorry to forgot about this PR (had little bit too much in the air) 😢
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r557459125
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveConf.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * For https://github.com/apache/iceberg/issues/2070
+ */
+public class TestHiveConf {
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void test() {
+ HiveConf hiveConf = createHiveConf();
Review comment:
@pvary the CI failed occasionally for org.apache.thrift.transport.TTransportException in spark-catalog, for example：
I think this may thrown by a hive metastore client, and maybe related to the code in org.apache.iceberg.hive.TestHiveMetastore#start(int), what do you think?
```java
// in Hive3, setting this as a system prop ensures that it will be picked up whenever a new HiveConf is created
System.setProperty(HiveConf.ConfVars.METASTOREURIS.varname, hiveConf.getVar(HiveConf.ConfVars.METASTOREURIS));
```
BTW, both Hive2 and Hive3 will pick up system property whenever a new HiveConf is created as i tested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592885852
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -45,6 +46,7 @@
public HiveClientPool(int poolSize, Configuration conf) {
super(poolSize, TTransportException.class);
this.hiveConf = new HiveConf(conf, HiveClientPool.class);
+ this.hiveConf.addResource(conf);
Review comment:
Hi peter, your code works fine under this test, but `new HiveConf(conf, HiveClientPool.class)` will copy all the `overlay` properties in `conf` and `new HiveConf(HiveClientPool.class)` will not.
In this test these two properties will be record in `overlay` properties (have higher priority):
```java
conf.set(HiveConf.ConfVars.METASTOREWAREHOUSE.varname, "file:/mywarehouse/");
conf.setInt("iceberg.hive.client-pool-size", 10);
```
So i think we should keep using `new HiveConf(conf, HiveClientPool.class)`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#issuecomment-796609864
@pvary @rdblue could you please review this pr again, i think this can help iceberg users who has enabled hive metastore sasl.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#issuecomment-801666233
@rdblue I see you've start marking issues in the 0.11.1 milestone, i think maybe this issue should be added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r592955587
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveClientPool.java
##########
@@ -45,6 +46,7 @@
public HiveClientPool(int poolSize, Configuration conf) {
super(poolSize, TTransportException.class);
this.hiveConf = new HiveConf(conf, HiveClientPool.class);
+ this.hiveConf.addResource(conf);
Review comment:
As i tested, `this.hiveConf.addResource(conf)` will not copy the overlay properties, the test can still pass, but `this.hiveConf.overlay` will not contains the origin overlay properties in `conf.overlay`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r557459125
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveConf.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * For https://github.com/apache/iceberg/issues/2070
+ */
+public class TestHiveConf {
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void test() {
+ HiveConf hiveConf = createHiveConf();
Review comment:
@pvary the CI failed occasionally for org.apache.thrift.transport.TTransportException in spark-catalog, for example：
I think this may thrown by a hive metastore client, and maybe related to the code (in `org.apache.iceberg.hive.TestHiveMetastore#start(int)`, i think `System.setProperty` under mutil-thread environment is not safe, what do you think?
```java
// in Hive3, setting this as a system prop ensures that it will be picked up whenever a new HiveConf is created
System.setProperty(HiveConf.ConfVars.METASTOREURIS.varname, hiveConf.getVar(HiveConf.ConfVars.METASTOREURIS));
```
BTW, both Hive2 and Hive3 will pick up system property whenever a new HiveConf is created as i tested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r557409606
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveConf.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * For https://github.com/apache/iceberg/issues/2070
+ */
+public class TestHiveConf {
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void test() {
+ HiveConf hiveConf = createHiveConf();
Review comment:
I added TestHiveClientPool, please take a look when you have spare time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r556284136
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveConf.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * For https://github.com/apache/iceberg/issues/2070
+ */
+public class TestHiveConf {
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void test() {
+ HiveConf hiveConf = createHiveConf();
Review comment:
Wouldn't it be better to have a test for the HiveClientPool specifically? We want to test that the clients in the pool has the specific configuration we need. What we doing here is testing that the HiveConf works as we expect. With some obscure change we can end up in a situation where the HiveConf behaviour does not change but the HMS Clients do not het the conf we need.
Thanks, Peter
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dixingxing0 commented on a change in pull request #2075: Avoid reset hive configuration to default value.
Posted by GitBox <gi...@apache.org>.
dixingxing0 commented on a change in pull request #2075:
URL: https://github.com/apache/iceberg/pull/2075#discussion_r557409606
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveConf.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.hive;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * For https://github.com/apache/iceberg/issues/2070
+ */
+public class TestHiveConf {
+ private static final String HIVE_SITE_CONTENT = "<?xml version=\"1.0\"?>\n" +
+ "<?xml-stylesheet type=\"text/xsl\" href=\"configuration.xsl\"?>\n" +
+ "<configuration>\n" +
+ " <property>\n" +
+ " <name>hive.metastore.sasl.enabled</name>\n" +
+ " <value>true</value>\n" +
+ " </property>\n" +
+ "</configuration>\n";
+
+ @Test
+ public void test() {
+ HiveConf hiveConf = createHiveConf();
Review comment:
I added TestHiveClientPool, please take a look when you have time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org