ajantha-bhat commented on code in PR #9282:
URL: https://github.com/apache/iceberg/pull/9282#discussion_r1423817274
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java:
##########
@@ -28,30 +28,46 @@
import java.util.Map;
import java.util.concurrent.TimeUnit;
import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.iceberg.CatalogProperties;
import org.apache.iceberg.CatalogUtil;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.hive.CachedClientPool.Key;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
-public class TestCachedClientPool extends HiveMetastoreTest {
+public class TestCachedClientPool {
+ private static final long EVICTION_INTERVAL = TimeUnit.SECONDS.toMillis(10);
+ private static final String DB_NAME = "hivedb";
+
+ @RegisterExtension
+ private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());
@Test
public void testClientPoolCleaner() throws InterruptedException {
- CachedClientPool clientPool = new CachedClientPool(hiveConf,
Collections.emptyMap());
+ CachedClientPool clientPool =
+ new CachedClientPool(
+ HIVE_METASTORE_EXTENSION.hiveConf(),
+ ImmutableMap.of(
+ CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
+ String.valueOf(EVICTION_INTERVAL)));
HiveClientPool clientPool1 = clientPool.clientPool();
assertThat(clientPool1)
.isSameAs(
CachedClientPool.clientPoolCache()
- .getIfPresent(CachedClientPool.extractKey(null, hiveConf)));
+ .getIfPresent(
+ CachedClientPool.extractKey(null,
HIVE_METASTORE_EXTENSION.hiveConf())));
TimeUnit.MILLISECONDS.sleep(EVICTION_INTERVAL -
TimeUnit.SECONDS.toMillis(2));
HiveClientPool clientPool2 = clientPool.clientPool();
assertThat(clientPool2).isSameAs(clientPool1);
TimeUnit.MILLISECONDS.sleep(EVICTION_INTERVAL +
TimeUnit.SECONDS.toMillis(5));
assertThat(
CachedClientPool.clientPoolCache()
- .getIfPresent(CachedClientPool.extractKey(null, hiveConf)))
+ .getIfPresent(
+ CachedClientPool.extractKey(null,
HIVE_METASTORE_EXTENSION.hiveConf())))
Review Comment:
nit: some test we are extracting to a variable and in some test we are using
directly. Let us have a unified style.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.util.Map;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Database;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public class HiveMetastoreExtension implements BeforeEachCallback,
AfterEachCallback {
+ private HiveMetaStoreClient metastoreClient;
+ private TestHiveMetastore metastore;
+ private final Map<String, String> hiveConfOverride;
+ private final String databaseName;
+
+ public HiveMetastoreExtension(String databaseName, Map<String, String>
hiveConfOverride) {
+ this.databaseName = databaseName;
+ this.hiveConfOverride = hiveConfOverride;
+ }
+
+ @Override
+ public void beforeEach(ExtensionContext extensionContext) throws Exception {
+ metastore = new TestHiveMetastore();
+ HiveConf hiveConfWithOverrides = new HiveConf(TestHiveMetastore.class);
+ if (hiveConfOverride != null) {
+ for (Map.Entry<String, String> kv : hiveConfOverride.entrySet()) {
+ hiveConfWithOverrides.set(kv.getKey(), kv.getValue());
+ }
+ }
+
+ metastore.start(hiveConfWithOverrides);
+ metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);
+
Review Comment:
nit: we can remove the new line
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommitLocks.java:
##########
@@ -88,13 +95,33 @@ public class TestHiveCommitLocks extends HiveTableBaseTest {
LockResponse notAcquiredLockResponse = new LockResponse(dummyLockId,
LockState.NOT_ACQUIRED);
ShowLocksResponse emptyLocks = new ShowLocksResponse(Lists.newArrayList());
- @BeforeAll
- public static void startMetastore() throws Exception {
- HiveMetastoreTest.startMetastore(
- ImmutableMap.of(HiveConf.ConfVars.HIVE_TXN_TIMEOUT.varname, "1s"));
-
+ private static final String DB_NAME = "hivedb";
+ private static final String TABLE_NAME = "tbl";
+ private static final Schema schema =
+ new Schema(Types.StructType.of(required(1, "id",
Types.LongType.get())).fields());
+ private static final PartitionSpec partitionSpec =
builderFor(schema).identity("id").build();
+ static final TableIdentifier TABLE_IDENTIFIER = TableIdentifier.of(DB_NAME,
TABLE_NAME);
+
+ @RegisterExtension
+ private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ new HiveMetastoreExtension(
+ DB_NAME, ImmutableMap.of(HiveConf.ConfVars.HIVE_TXN_TIMEOUT.varname,
"1s"));
+
+ private HiveCatalog catalog;
+
+ private void startMetastore() throws Exception {
Review Comment:
this method is not starting the `metastore`
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java:
##########
@@ -60,8 +66,23 @@ public class HiveCreateReplaceTableTest extends
HiveMetastoreTest {
private String tableLocation;
+ @RegisterExtension
+ private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());
+
+ private HiveCatalog catalog;
+
@BeforeEach
public void createTableLocation() throws IOException {
+ catalog =
Review Comment:
Also can we move this to `HiveMetastoreExtension` and get the catalog from
there? It can help in duplicating this code in each test class which uses this
extension.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
