Skip to content

Commit

Permalink
Merge pull request #1 from nastra/review_hive_tests_refactor
Browse files Browse the repository at this point in the history 
Improvements
  • Loading branch information
@nk1506
nk1506 authored Oct 31, 2023
2 parents 0c54560 + dab3be1 commit c9ef7d9
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 46 deletions.
52 changes: 24 additions & 28 deletions hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,22 @@
package org.apache.iceberg.hive;

import java.util.Map;
import java.util.concurrent.TimeUnit;
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.CatalogProperties;
import org.apache.iceberg.CatalogUtil;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
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 final class HiveMetastoreExtension implements AfterEachCallback, BeforeEachCallback {

static HiveCatalog catalog;
static HiveMetaStoreClient metastoreClient;
static TestHiveMetastore metastore;
static HiveConf hiveConf;
public class HiveMetastoreExtension implements BeforeEachCallback, AfterEachCallback {
private HiveMetaStoreClient metastoreClient;
private TestHiveMetastore metastore;
private final Map<String, String> hiveConfOverride;
static final String DB_NAME = "hivedb";
private final String databaseName;

public HiveMetastoreExtension(Map<String, String> hiveConfOverride) {
public HiveMetastoreExtension(String databaseName, Map<String, String> hiveConfOverride) {
this.databaseName = databaseName;
this.hiveConfOverride = hiveConfOverride;
}

Expand All @@ -55,30 +49,32 @@ public void beforeEach(ExtensionContext extensionContext) throws Exception {
}

metastore.start(hiveConfWithOverrides);
hiveConf = metastore.hiveConf();
metastoreClient = new HiveMetaStoreClient(hiveConfWithOverrides);

String dbPath = metastore.getDatabasePath(DB_NAME);
Database db = new Database(DB_NAME, "description", dbPath, Maps.newHashMap());
String dbPath = metastore.getDatabasePath(databaseName);
Database db = new Database(databaseName, "description", dbPath, Maps.newHashMap());
metastoreClient.createDatabase(db);

catalog =
(HiveCatalog)
CatalogUtil.loadCatalog(
HiveCatalog.class.getName(),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE,
ImmutableMap.of(
CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
String.valueOf(TimeUnit.SECONDS.toMillis(10))),
hiveConfWithOverrides);
}

@Override
public void afterEach(ExtensionContext extensionContext) throws Exception {
catalog = null;
metastoreClient.close();
if (null != metastoreClient) {
metastoreClient.close();
}

if (null != metastore) {
metastore.stop();
}

metastoreClient = null;
metastore.stop();
metastore = null;
}

public HiveMetaStoreClient metastoreClient() {
return metastoreClient;
}

public HiveConf hiveConf() {
return metastore.hiveConf();
}
}
2 changes: 1 addition & 1 deletion hive-metastore/src/test/java/org/apache/iceberg/hive/HiveMetastoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
import org.junit.jupiter.api.BeforeAll;

/*
* This meta-setup has been deprecated use {@link HiveMetastoreExtension} instead.
* This meta-setup has been deprecated; use {@link HiveMetastoreExtension} instead.
* */
@Deprecated
public abstract class HiveMetastoreTest {
Expand Down
53 changes: 36 additions & 17 deletions hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@
import static org.apache.iceberg.TableProperties.DEFAULT_SORT_ORDER;
import static org.apache.iceberg.TableProperties.SNAPSHOT_COUNT;
import static org.apache.iceberg.expressions.Expressions.bucket;
import static org.apache.iceberg.hive.HiveMetastoreExtension.DB_NAME;
import static org.apache.iceberg.hive.HiveMetastoreExtension.catalog;
import static org.apache.iceberg.hive.HiveMetastoreExtension.hiveConf;
import static org.apache.iceberg.hive.HiveMetastoreExtension.metastoreClient;
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatNoException;
Expand All @@ -46,6 +42,7 @@
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.TimeUnit;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.metastore.api.Database;
Expand Down Expand Up @@ -88,6 +85,7 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.JsonUtil;
import org.apache.thrift.TException;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.api.io.TempDir;
Expand All @@ -109,9 +107,25 @@ public class TestHiveCatalog extends CatalogTests<HiveCatalog> {

@TempDir private Path temp;

private HiveCatalog catalog;
private static final String DB_NAME = "hivedb";

@RegisterExtension
public static final HiveMetastoreExtension hiveMetastoreExtension =
new HiveMetastoreExtension(Collections.emptyMap());
private static final HiveMetastoreExtension HIVE_METASTORE_EXTENSION =
new HiveMetastoreExtension(DB_NAME, Collections.emptyMap());

@BeforeEach
public void setup() {
catalog =
(HiveCatalog)
CatalogUtil.loadCatalog(
HiveCatalog.class.getName(),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE,
ImmutableMap.of(
CatalogProperties.CLIENT_POOL_CACHE_EVICTION_INTERVAL_MS,
String.valueOf(TimeUnit.SECONDS.toMillis(10))),
HIVE_METASTORE_EXTENSION.hiveConf());
}

@Override
protected boolean requiresNamespaceCreate() {
Expand Down Expand Up @@ -329,7 +343,8 @@ private void createTableAndVerifyOwner(
String location = temp.resolve(tbl).toString();
try {
Table table = catalog.createTable(tableIdent, schema, spec, location, properties);
org.apache.hadoop.hive.metastore.api.Table hmsTable = metastoreClient.getTable(db, tbl);
org.apache.hadoop.hive.metastore.api.Table hmsTable =
HIVE_METASTORE_EXTENSION.metastoreClient().getTable(db, tbl);
assertThat(hmsTable.getOwner()).isEqualTo(owner);
Map<String, String> hmsTableParams = hmsTable.getParameters();
assertThat(hmsTableParams).doesNotContainKey(HiveCatalog.HMS_TABLE_OWNER);
Expand Down Expand Up @@ -394,7 +409,8 @@ public void testCreateTableCustomSortOrder() throws Exception {
public void testDatabaseAndNamespaceWithLocation() throws Exception {
Namespace namespace1 = Namespace.of("noLocation");
catalog.createNamespace(namespace1, meta);
Database database1 = metastoreClient.getDatabase(namespace1.toString());
Database database1 =
HIVE_METASTORE_EXTENSION.metastoreClient().getDatabase(namespace1.toString());

assertThat(database1.getParameters()).containsEntry("owner", "apache");
assertThat(database1.getParameters()).containsEntry("group", "iceberg");
Expand All @@ -417,7 +433,8 @@ public void testDatabaseAndNamespaceWithLocation() throws Exception {
Namespace namespace2 = Namespace.of("haveLocation");

catalog.createNamespace(namespace2, newMeta);
Database database2 = metastoreClient.getDatabase(namespace2.toString());
Database database2 =
HIVE_METASTORE_EXTENSION.metastoreClient().getDatabase(namespace2.toString());
assertThat(hiveLocalDir)
.as("There no same location for db and namespace")
.isEqualTo(database2.getLocationUri());
Expand Down Expand Up @@ -497,7 +514,7 @@ private void createNamespaceAndVerifyOwnership(
Namespace namespace = Namespace.of(name);

catalog.createNamespace(namespace, prop);
Database db = metastoreClient.getDatabase(namespace.toString());
Database db = HIVE_METASTORE_EXTENSION.metastoreClient().getDatabase(namespace.toString());

assertThat(db.getOwnerName()).isEqualTo(expectedOwner);
assertThat(db.getOwnerType()).isEqualTo(expectedOwnerType);
Expand Down Expand Up @@ -719,7 +736,7 @@ private void setNamespaceOwnershipAndVerify(
name, propToCreate, expectedOwnerPostCreate, expectedOwnerTypePostCreate);

catalog.setProperties(Namespace.of(name), propToSet);
Database database = metastoreClient.getDatabase(name);
Database database = HIVE_METASTORE_EXTENSION.metastoreClient().getDatabase(name);

assertThat(database.getOwnerName()).isEqualTo(expectedOwnerPostSet);
assertThat(database.getOwnerType()).isEqualTo(expectedOwnerTypePostSet);
Expand Down Expand Up @@ -844,15 +861,14 @@ private void removeNamespaceOwnershipAndVerify(

catalog.removeProperties(Namespace.of(name), propToRemove);

Database database = metastoreClient.getDatabase(name);
Database database = HIVE_METASTORE_EXTENSION.metastoreClient().getDatabase(name);

assertThat(database.getOwnerName()).isEqualTo(expectedOwnerPostRemove);
assertThat(database.getOwnerType()).isEqualTo(expectedOwnerTypePostRemove);
}

@Test
@Override
public void testDropNamespace() {
public void dropNamespace() {
Namespace namespace = Namespace.of("dbname_drop");
TableIdentifier identifier = TableIdentifier.of(namespace, "table");
Schema schema = getTestSchema();
Expand Down Expand Up @@ -915,7 +931,9 @@ public void testTableName() {
}

private String defaultUri(Namespace namespace) throws TException {
return metastoreClient.getConfigValue("hive.metastore.warehouse.dir", "")
return HIVE_METASTORE_EXTENSION
.metastoreClient()
.getConfigValue("hive.metastore.warehouse.dir", "")
+ "/"
+ namespace.level(0)
+ ".db";
Expand Down Expand Up @@ -1091,7 +1109,8 @@ public void testSetCurrentSchema() throws Exception {
}

private Map<String, String> hmsTableParameters() throws TException {
org.apache.hadoop.hive.metastore.api.Table hmsTable = metastoreClient.getTable(DB_NAME, "tbl");
org.apache.hadoop.hive.metastore.api.Table hmsTable =
HIVE_METASTORE_EXTENSION.metastoreClient().getTable(DB_NAME, "tbl");
return hmsTable.getParameters();
}

Expand Down Expand Up @@ -1124,7 +1143,7 @@ public void testTablePropsDefinedAtCatalogLevel() {
HiveCatalog.class.getName(),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE,
catalogProps,
hiveConf);
HIVE_METASTORE_EXTENSION.hiveConf());

try {
Table table =
Expand Down

0 comments on commit c9ef7d9

Please sign in to comment.