nastra commented on code in PR #9282:
URL: https://github.com/apache/iceberg/pull/9282#discussion_r1423567661
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableBaseTest.java:
##########
@@ -26,22 +26,34 @@
import java.io.File;
import java.nio.file.Paths;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
+import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import org.apache.hadoop.fs.Path;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.CatalogUtil;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.TableMetadataParser;
import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
-import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.RegisterExtension;
-public class HiveTableBaseTest extends HiveMetastoreTest {
+public class HiveTableBaseTest {
static final String TABLE_NAME = "tbl";
+ static final String DB_NAME = "hivedb";
static final TableIdentifier TABLE_IDENTIFIER = TableIdentifier.of(DB_NAME,
TABLE_NAME);
+ @RegisterExtension
+ static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());
+
+ HiveCatalog catalog;
Review Comment:
```suggestion
protected HiveCatalog catalog;
```
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java:
##########
@@ -28,17 +28,41 @@
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.BeforeEach;
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 HiveConf hiveConf;
+ private static final String DB_NAME = "hivedb";
+
+ @RegisterExtension
+ private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());
+
+ @BeforeEach
+ public void initialize() {
+ hiveConf = HIVE_METASTORE_EXTENSION.hiveConf();
Review Comment:
why are we initializing the config like that? In all the other places the
config is just used directly from the extension:
`HIVE_METASTORE_EXTENSION.hiveConf()`
##########
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;
+
+ public void startMetastore() throws Exception {
Review Comment:
```suggestion
private void startMetastore() throws Exception {
```
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java:
##########
@@ -28,17 +28,41 @@
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.BeforeEach;
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 HiveConf hiveConf;
+ private static final String DB_NAME = "hivedb";
+
+ @RegisterExtension
+ private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());
+
+ @BeforeEach
+ public void initialize() {
+ hiveConf = HIVE_METASTORE_EXTENSION.hiveConf();
+ }
@Test
public void testClientPoolCleaner() throws InterruptedException {
+ CatalogUtil.loadCatalog(
+ HiveCatalog.class.getName(),
+ CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE,
+ ImmutableMap.of(
+ CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
+ String.valueOf(EVICTION_INTERVAL)),
+ hiveConf);
Review Comment:
```suggestion
HIVE_METASTORE_EXTENSION.hiveConf());
```
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java:
##########
@@ -28,17 +28,41 @@
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.BeforeEach;
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);
Review Comment:
can you make this please consistent with other tests? Other tests don't
define a constant but directly use 10 millis in the init code
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableBaseTest.java:
##########
@@ -26,22 +26,34 @@
import java.io.File;
import java.nio.file.Paths;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
+import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import org.apache.hadoop.fs.Path;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.CatalogUtil;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.TableMetadataParser;
import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
-import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.RegisterExtension;
-public class HiveTableBaseTest extends HiveMetastoreTest {
+public class HiveTableBaseTest {
static final String TABLE_NAME = "tbl";
+ static final String DB_NAME = "hivedb";
static final TableIdentifier TABLE_IDENTIFIER = TableIdentifier.of(DB_NAME,
TABLE_NAME);
+ @RegisterExtension
+ static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
Review Comment:
```suggestion
protected static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
```
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestCachedClientPool.java:
##########
@@ -28,17 +28,41 @@
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.BeforeEach;
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 HiveConf hiveConf;
+ private static final String DB_NAME = "hivedb";
+
+ @RegisterExtension
+ private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
+ new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());
+
+ @BeforeEach
+ public void initialize() {
+ hiveConf = HIVE_METASTORE_EXTENSION.hiveConf();
+ }
@Test
public void testClientPoolCleaner() throws InterruptedException {
+ CatalogUtil.loadCatalog(
+ HiveCatalog.class.getName(),
+ CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE,
+ ImmutableMap.of(
+ CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
+ String.valueOf(EVICTION_INTERVAL)),
Review Comment:
```suggestion
String.valueOf(TimeUnit.SECONDS.toMillis(10))),
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
