From 8d582473859c3aff18b9b8c9c16e8224d639ed99 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Fri, 6 Sep 2024 11:27:35 -0700 Subject: [PATCH 01/32] Add REST Catalog tests to Spark 3.5 integration test Add REST Catalog tests to Spark 3.4 integration test tmp save Fix integ tests Revert "Add REST Catalog tests to Spark 3.4 integration test" This reverts commit d052416d4e8a2c26ece3d250e1b213a41a7f7cdd. unneeded changes fix test retrigger checks Fix integ test Fix port already in use Fix unmatched validation catalog spotless Fix sqlite related test failures --- .../org/apache/iceberg/rest/RCKUtils.java | 10 ++- .../iceberg/rest/RESTCatalogServer.java | 7 +- spark/v3.5/build.gradle | 22 ++++++ .../spark/extensions/TestMetadataTables.java | 15 +++- .../TestRemoveOrphanFilesProcedure.java | 12 +-- .../apache/iceberg/spark/CatalogTestBase.java | 10 +++ .../iceberg/spark/SparkCatalogConfig.java | 4 + .../iceberg/spark/TestBaseWithCatalog.java | 77 +++++++++++++++++-- .../actions/TestComputeTableStatsAction.java | 1 + .../iceberg/spark/sql/TestAlterTable.java | 8 ++ .../iceberg/spark/sql/TestRefreshTable.java | 8 +- 11 files changed, 154 insertions(+), 20 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java index 0f1571d362e3..1c2a265137b6 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java @@ -29,7 +29,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.util.PropertyUtil; -class RCKUtils { +public class RCKUtils { private static final String CATALOG_ENV_PREFIX = "CATALOG_"; static final String RCK_LOCAL = "rck.local"; static final String RCK_PURGE_TEST_NAMESPACES = "rck.purge-test-namespaces"; @@ -76,15 +76,17 @@ static Map environmentCatalogConfig() { HashMap::new)); } - static RESTCatalog initCatalogClient() { + public static RESTCatalog initCatalogClient() { Map catalogProperties = Maps.newHashMap(); catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); catalogProperties.putAll(Maps.fromProperties(System.getProperties())); // Set defaults + String port = + catalogProperties.getOrDefault( + RESTCatalogServer.REST_PORT, String.valueOf(RESTCatalogServer.REST_PORT_DEFAULT)); catalogProperties.putIfAbsent( - CatalogProperties.URI, - String.format("http://localhost:%s/", RESTCatalogServer.REST_PORT_DEFAULT)); + CatalogProperties.URI, String.format("http://localhost:%s/", port)); catalogProperties.putIfAbsent(CatalogProperties.WAREHOUSE_LOCATION, "rck_warehouse"); RESTCatalog catalog = new RESTCatalog(); diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java index b3d12f74e4b0..31ea1b696a3b 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java @@ -26,6 +26,7 @@ import org.apache.iceberg.CatalogUtil; import org.apache.iceberg.catalog.Catalog; import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.util.PropertyUtil; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.gzip.GzipHandler; @@ -42,7 +43,7 @@ public class RESTCatalogServer { private Server httpServer; - RESTCatalogServer() {} + public RESTCatalogServer() {} static class CatalogContext { private final Catalog catalog; @@ -64,7 +65,9 @@ public Map configuration() { private CatalogContext initializeBackendCatalog() throws IOException { // Translate environment variables to catalog properties - Map catalogProperties = RCKUtils.environmentCatalogConfig(); + Map catalogProperties = Maps.newHashMap(); + catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); + catalogProperties.putAll(Maps.fromProperties(System.getProperties())); // Fallback to a JDBCCatalog impl if one is not set catalogProperties.putIfAbsent(CatalogProperties.CATALOG_IMPL, JdbcCatalog.class.getName()); diff --git a/spark/v3.5/build.gradle b/spark/v3.5/build.gradle index e3c9ef4f0230..cfb5912a774f 100644 --- a/spark/v3.5/build.gradle +++ b/spark/v3.5/build.gradle @@ -107,8 +107,13 @@ project(":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}") { testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts') testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts') testImplementation project(path: ':iceberg-data', configuration: 'testArtifacts') + testImplementation (project(path: ':iceberg-open-api', configuration: 'testFixturesRuntimeElements')) { + transitive = false + } testImplementation libs.sqlite.jdbc testImplementation libs.awaitility + // runtime dependencies for running REST Catalog based integration test + testRuntimeOnly libs.jetty.servlet } test { @@ -172,6 +177,12 @@ project(":iceberg-spark:iceberg-spark-extensions-${sparkMajorVersion}_${scalaVer testImplementation project(path: ':iceberg-core', configuration: 'testArtifacts') testImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts') testImplementation project(path: ":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}", configuration: 'testArtifacts') + testImplementation (project(path: ':iceberg-open-api', configuration: 'testFixturesRuntimeElements')) { + transitive = false + } + // runtime dependencies for running REST Catalog based integration test + testRuntimeOnly libs.jetty.servlet + testRuntimeOnly libs.sqlite.jdbc testImplementation libs.avro.avro testImplementation libs.parquet.hadoop @@ -255,6 +266,17 @@ project(":iceberg-spark:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersio integrationImplementation project(path: ':iceberg-hive-metastore', configuration: 'testArtifacts') integrationImplementation project(path: ":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}", configuration: 'testArtifacts') integrationImplementation project(path: ":iceberg-spark:iceberg-spark-extensions-${sparkMajorVersion}_${scalaVersion}", configuration: 'testArtifacts') + + // runtime dependencies for running Hive Catalog based integration test + integrationRuntimeOnly project(':iceberg-hive-metastore') + // runtime dependencies for running REST Catalog based integration test + integrationRuntimeOnly project(path: ':iceberg-core', configuration: 'testArtifacts') + integrationRuntimeOnly (project(path: ':iceberg-open-api', configuration: 'testFixturesRuntimeElements')) { + transitive = false + } + integrationRuntimeOnly libs.jetty.servlet + integrationRuntimeOnly libs.sqlite.jdbc + // Not allowed on our classpath, only the runtime jar is allowed integrationCompileOnly project(":iceberg-spark:iceberg-spark-extensions-${sparkMajorVersion}_${scalaVersion}") integrationCompileOnly project(":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}") diff --git a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java index a22cf61ec8c9..b451575a4fe0 100644 --- a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java +++ b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java @@ -18,12 +18,15 @@ */ package org.apache.iceberg.spark.extensions; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_REST; import static org.apache.iceberg.types.Types.NestedField.optional; import static org.assertj.core.api.Assertions.assertThat; import java.io.IOException; import java.util.Comparator; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.avro.generic.GenericData.Record; @@ -400,6 +403,11 @@ public void testAllFilesPartitioned() throws Exception { @TestTemplate public void testMetadataLogEntries() throws Exception { + if (Set.of(ICEBERG_CATALOG_TYPE_REST).contains(catalogConfig.get(ICEBERG_CATALOG_TYPE))) { + // need to fix https://github.com/apache/iceberg/issues/11103 before enabling this test on + // rest catalog + return; + } // Create table and insert data sql( "CREATE TABLE %s (id bigint, data string) " @@ -521,7 +529,7 @@ public void testFilesTableTimeTravelWithSchemaEvolution() throws Exception { optional(3, "category", Types.StringType.get()))); spark.createDataFrame(newRecords, newSparkSchema).coalesce(1).writeTo(tableName).append(); - + table.refresh(); Long currentSnapshotId = table.currentSnapshot().snapshotId(); Dataset actualFilesDs = @@ -740,6 +748,11 @@ private boolean partitionMatch(Record file, String partValue) { @TestTemplate public void metadataLogEntriesAfterReplacingTable() throws Exception { + if (Set.of(ICEBERG_CATALOG_TYPE_REST).contains(catalogConfig.get(ICEBERG_CATALOG_TYPE))) { + // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on + // rest catalog + return; + } sql( "CREATE TABLE %s (id bigint, data string) " + "USING iceberg " diff --git a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java index d8feaa77079b..7731c613a6ba 100644 --- a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java +++ b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java @@ -450,12 +450,14 @@ public void testRemoveOrphanFilesWithStatisticFiles() throws Exception { Table table = Spark3Util.loadIcebergTable(spark, tableName); String statsFileName = "stats-file-" + UUID.randomUUID(); + String location = table.location(); + // not every catalog will return file proto for local directories + // i.e. Hadoop and Hive Catalog does, Jdbc and REST does not + if (!location.startsWith("file:")) { + location = "file:" + location; + } File statsLocation = - new File(new URI(table.location())) - .toPath() - .resolve("data") - .resolve(statsFileName) - .toFile(); + new File(new URI(location)).toPath().resolve("data").resolve(statsFileName).toFile(); StatisticsFile statisticsFile; try (PuffinWriter puffinWriter = Puffin.write(Files.localOutput(statsLocation)).build()) { long snapshotId = table.currentSnapshot().snapshotId(); diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java index ba864bf89e33..395947fa21b5 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java @@ -18,6 +18,8 @@ */ package org.apache.iceberg.spark; +import com.google.common.collect.ImmutableMap; +import org.apache.iceberg.CatalogProperties; import org.apache.iceberg.ParameterizedTestExtension; import org.apache.iceberg.Parameters; import org.junit.jupiter.api.extension.ExtendWith; @@ -43,6 +45,14 @@ protected static Object[][] parameters() { SparkCatalogConfig.SPARK.catalogName(), SparkCatalogConfig.SPARK.implementation(), SparkCatalogConfig.SPARK.properties() + }, + { + SparkCatalogConfig.REST.catalogName(), + SparkCatalogConfig.REST.implementation(), + ImmutableMap.builder() + .putAll(SparkCatalogConfig.REST.properties()) + .put(CatalogProperties.URI, restCatalog.properties().get(CatalogProperties.URI)) + .build() } }; } diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogConfig.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogConfig.java index abfd7da0c7bd..e03f500dc46c 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogConfig.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogConfig.java @@ -34,6 +34,10 @@ public enum SparkCatalogConfig { "testhadoop", SparkCatalog.class.getName(), ImmutableMap.of("type", "hadoop", "cache-enabled", "false")), + REST( + "testrest", + SparkCatalog.class.getName(), + ImmutableMap.of("type", "rest", "cache-enabled", "false")), SPARK( "spark_catalog", SparkSessionCatalog.class.getName(), diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index c869c4a30a19..dc804968df2d 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -18,6 +18,11 @@ */ package org.apache.iceberg.spark; +import static org.apache.iceberg.CatalogProperties.CATALOG_IMPL; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_HADOOP; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_REST; import static org.assertj.core.api.Assertions.assertThat; import java.io.File; @@ -25,6 +30,7 @@ import java.util.Map; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hive.metastore.MetaStoreUtils; import org.apache.iceberg.CatalogProperties; import org.apache.iceberg.Parameter; import org.apache.iceberg.ParameterizedTestExtension; @@ -36,6 +42,10 @@ import org.apache.iceberg.catalog.SupportsNamespaces; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.hadoop.HadoopCatalog; +import org.apache.iceberg.inmemory.InMemoryCatalog; +import org.apache.iceberg.rest.RCKUtils; +import org.apache.iceberg.rest.RESTCatalog; +import org.apache.iceberg.rest.RESTCatalogServer; import org.apache.iceberg.util.PropertyUtil; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -46,6 +56,8 @@ @ExtendWith(ParameterizedTestExtension.class) public abstract class TestBaseWithCatalog extends TestBase { protected static File warehouse = null; + protected static RESTCatalogServer restServer; + protected static RESTCatalog restCatalog; @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}") protected static Object[][] parameters() { @@ -59,18 +71,47 @@ protected static Object[][] parameters() { } @BeforeAll - public static void createWarehouse() throws IOException { + public static void createWarehouseAndStartRest() throws IOException { TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null); assertThat(warehouse.delete()).isTrue(); + try { + restServer = new RESTCatalogServer(); + // prevent using already-in-use port when testing + System.setProperty("rest.port", String.valueOf(MetaStoreUtils.findFreePort())); + System.setProperty(CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath()); + // In-memory sqlite database by default is private to the connection that created it. + // If more than 1 jdbc connection backed by in-memory sqlite is created behind one + // JdbcCatalog, then different jdbc connections could provide different views of table + // status even belonging to the same catalog. Reference: + // https://www.sqlite.org/inmemorydb.html + System.setProperty(CatalogProperties.CLIENT_POOL_SIZE, "1"); + restServer.start(false); + restCatalog = RCKUtils.initCatalogClient(); + System.clearProperty("rest.port"); + System.clearProperty(CatalogProperties.WAREHOUSE_LOCATION); + System.clearProperty(CatalogProperties.CLIENT_POOL_SIZE); + } catch (Exception e) { + throw new RuntimeException(e); + } } @AfterAll - public static void dropWarehouse() throws IOException { + public static void dropWarehouseAndStopRest() throws IOException { if (warehouse != null && warehouse.exists()) { Path warehousePath = new Path(warehouse.getAbsolutePath()); FileSystem fs = warehousePath.getFileSystem(hiveConf); assertThat(fs.delete(warehousePath, true)).as("Failed to delete " + warehousePath).isTrue(); } + try { + if (restCatalog != null) { + restCatalog.close(); + } + if (restServer != null) { + restServer.stop(); + } + } catch (Exception e) { + throw new RuntimeException(e); + } } @TempDir protected java.nio.file.Path temp; @@ -91,10 +132,30 @@ public static void dropWarehouse() throws IOException { @BeforeEach public void before() { - this.validationCatalog = - catalogName.equals("testhadoop") - ? new HadoopCatalog(spark.sessionState().newHadoopConf(), "file:" + warehouse) - : catalog; + if (catalogConfig.containsKey(ICEBERG_CATALOG_TYPE)) { + switch (catalogConfig.get(ICEBERG_CATALOG_TYPE)) { + case ICEBERG_CATALOG_TYPE_HADOOP: + this.validationCatalog = + new HadoopCatalog(spark.sessionState().newHadoopConf(), "file:" + warehouse); + break; + case ICEBERG_CATALOG_TYPE_REST: + this.validationCatalog = restCatalog; + break; + case ICEBERG_CATALOG_TYPE_HIVE: + this.validationCatalog = catalog; + break; + default: + throw new IllegalArgumentException("Unknown catalog type"); + } + } else if (catalogConfig.containsKey(CATALOG_IMPL)) { + switch (catalogConfig.get(CATALOG_IMPL)) { + case "org.apache.iceberg.inmemory.InMemoryCatalog": + this.validationCatalog = new InMemoryCatalog(); + break; + default: + throw new IllegalArgumentException("Unknown catalog impl"); + } + } this.validationNamespaceCatalog = (SupportsNamespaces) validationCatalog; spark.conf().set("spark.sql.catalog." + catalogName, implementation); @@ -105,6 +166,10 @@ public void before() { spark.conf().set("spark.sql.catalog." + catalogName + ".warehouse", "file:" + warehouse); } + if (!catalogName.equals("spark_catalog")) { + spark.conf().set("spark.sql.default.catalog", catalogName); + } + this.tableName = (catalogName.equals("spark_catalog") ? "" : catalogName + ".") + "default.table"; diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestComputeTableStatsAction.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestComputeTableStatsAction.java index 7aa849d0bba8..057ef231ca1d 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestComputeTableStatsAction.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestComputeTableStatsAction.java @@ -110,6 +110,7 @@ public void testComputeTableStatsAction() throws NoSuchTableException, ParseExce new SimpleRecord(4, "d")); spark.createDataset(records, Encoders.bean(SimpleRecord.class)).writeTo(tableName).append(); SparkActions actions = SparkActions.get(); + table.refresh(); ComputeTableStats.Result results = actions.computeTableStats(table).columns("id", "data").execute(); assertThat(results).isNotNull(); diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java index 7c98888f1667..725f34693920 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java @@ -18,10 +18,13 @@ */ package org.apache.iceberg.spark.sql; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_REST; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assumptions.assumeThat; +import java.util.Set; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.hadoop.HadoopCatalog; @@ -275,6 +278,11 @@ public void testAlterColumnPositionFirst() { @TestTemplate public void testTableRename() { + if (Set.of(ICEBERG_CATALOG_TYPE_REST).contains(catalogConfig.get(ICEBERG_CATALOG_TYPE))) { + // need to fix https://github.com/apache/iceberg/issues/11154 before enabling this test on + // rest catalog + return; + } assumeThat(validationCatalog) .as("Hadoop catalog does not support rename") .isNotInstanceOf(HadoopCatalog.class); diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestRefreshTable.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestRefreshTable.java index 8a9ae0f6030a..fe13d61db066 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestRefreshTable.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestRefreshTable.java @@ -19,6 +19,7 @@ package org.apache.iceberg.spark.sql; import java.util.List; +import java.util.Set; import org.apache.iceberg.DataFile; import org.apache.iceberg.Table; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; @@ -45,8 +46,11 @@ public void removeTables() { public void testRefreshCommand() { // We are not allowed to change the session catalog after it has been initialized, so build a // new one - if (catalogName.equals(SparkCatalogConfig.SPARK.catalogName()) - || catalogName.equals(SparkCatalogConfig.HADOOP.catalogName())) { + if (Set.of( + SparkCatalogConfig.SPARK.catalogName(), + SparkCatalogConfig.HADOOP.catalogName(), + SparkCatalogConfig.REST.catalogName()) + .contains(catalogName)) { spark.conf().set("spark.sql.catalog." + catalogName + ".cache-enabled", true); spark = spark.cloneSession(); } From aad8b27c3342eb4655809f6cf19cb2b1f111b99f Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Wed, 18 Sep 2024 16:18:36 -0700 Subject: [PATCH 02/32] Rebase & spotless --- .../apache/iceberg/spark/extensions/TestMetadataTables.java | 5 ----- .../test/java/org/apache/iceberg/spark/CatalogTestBase.java | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java index b451575a4fe0..26e80b29a27a 100644 --- a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java +++ b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java @@ -403,11 +403,6 @@ public void testAllFilesPartitioned() throws Exception { @TestTemplate public void testMetadataLogEntries() throws Exception { - if (Set.of(ICEBERG_CATALOG_TYPE_REST).contains(catalogConfig.get(ICEBERG_CATALOG_TYPE))) { - // need to fix https://github.com/apache/iceberg/issues/11103 before enabling this test on - // rest catalog - return; - } // Create table and insert data sql( "CREATE TABLE %s (id bigint, data string) " diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java index 395947fa21b5..6cc100097c7a 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/CatalogTestBase.java @@ -18,10 +18,10 @@ */ package org.apache.iceberg.spark; -import com.google.common.collect.ImmutableMap; import org.apache.iceberg.CatalogProperties; import org.apache.iceberg.ParameterizedTestExtension; import org.apache.iceberg.Parameters; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.extension.ExtendWith; @ExtendWith(ParameterizedTestExtension.class) From 18d7c8a82ee94fb631b14b3c23948cc02e9ee53d Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 10 Oct 2024 12:54:22 -0700 Subject: [PATCH 03/32] code format --- .../spark/extensions/TestMetadataTables.java | 11 +++---- .../iceberg/spark/TestBaseWithCatalog.java | 32 +++++++++++++------ .../iceberg/spark/sql/TestAlterTable.java | 10 +++--- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java index 26e80b29a27a..26f8e0d6ba08 100644 --- a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java +++ b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java @@ -22,11 +22,11 @@ import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_REST; import static org.apache.iceberg.types.Types.NestedField.optional; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assumptions.assumeFalse; import java.io.IOException; import java.util.Comparator; import java.util.List; -import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.avro.generic.GenericData.Record; @@ -743,11 +743,10 @@ private boolean partitionMatch(Record file, String partValue) { @TestTemplate public void metadataLogEntriesAfterReplacingTable() throws Exception { - if (Set.of(ICEBERG_CATALOG_TYPE_REST).contains(catalogConfig.get(ICEBERG_CATALOG_TYPE))) { - // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on - // rest catalog - return; - } + // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on + // rest catalog + assumeFalse(ICEBERG_CATALOG_TYPE_REST.equals(catalogConfig.get(ICEBERG_CATALOG_TYPE))); + sql( "CREATE TABLE %s (id bigint, data string) " + "USING iceberg " diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index dc804968df2d..4add66c567bc 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -71,9 +71,23 @@ protected static Object[][] parameters() { } @BeforeAll - public static void createWarehouseAndStartRest() throws IOException { + public static void setUpAll() throws IOException { TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null); assertThat(warehouse.delete()).isTrue(); + startRESTServer(); + } + + @AfterAll + public static void tearDownAll() throws IOException { + if (warehouse != null && warehouse.exists()) { + Path warehousePath = new Path(warehouse.getAbsolutePath()); + FileSystem fs = warehousePath.getFileSystem(hiveConf); + assertThat(fs.delete(warehousePath, true)).as("Failed to delete " + warehousePath).isTrue(); + } + stopRESTServer(); + } + + private static void startRESTServer() { try { restServer = new RESTCatalogServer(); // prevent using already-in-use port when testing @@ -95,13 +109,7 @@ public static void createWarehouseAndStartRest() throws IOException { } } - @AfterAll - public static void dropWarehouseAndStopRest() throws IOException { - if (warehouse != null && warehouse.exists()) { - Path warehousePath = new Path(warehouse.getAbsolutePath()); - FileSystem fs = warehousePath.getFileSystem(hiveConf); - assertThat(fs.delete(warehousePath, true)).as("Failed to delete " + warehousePath).isTrue(); - } + private static void stopRESTServer() { try { if (restCatalog != null) { restCatalog.close(); @@ -130,8 +138,7 @@ public static void dropWarehouseAndStopRest() throws IOException { protected TableIdentifier tableIdent = TableIdentifier.of(Namespace.of("default"), "table"); protected String tableName; - @BeforeEach - public void before() { + private void configureValidationCatalog() { if (catalogConfig.containsKey(ICEBERG_CATALOG_TYPE)) { switch (catalogConfig.get(ICEBERG_CATALOG_TYPE)) { case ICEBERG_CATALOG_TYPE_HADOOP: @@ -157,6 +164,11 @@ public void before() { } } this.validationNamespaceCatalog = (SupportsNamespaces) validationCatalog; + } + + @BeforeEach + public void before() { + configureValidationCatalog(); spark.conf().set("spark.sql.catalog." + catalogName, implementation); catalogConfig.forEach( diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java index 725f34693920..0665427f9e8b 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java @@ -23,8 +23,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assumptions.assumeThat; +import static org.junit.jupiter.api.Assumptions.assumeFalse; -import java.util.Set; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.hadoop.HadoopCatalog; @@ -278,11 +278,9 @@ public void testAlterColumnPositionFirst() { @TestTemplate public void testTableRename() { - if (Set.of(ICEBERG_CATALOG_TYPE_REST).contains(catalogConfig.get(ICEBERG_CATALOG_TYPE))) { - // need to fix https://github.com/apache/iceberg/issues/11154 before enabling this test on - // rest catalog - return; - } + // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on + // rest catalog + assumeFalse(ICEBERG_CATALOG_TYPE_REST.equals(catalogConfig.get(ICEBERG_CATALOG_TYPE))); assumeThat(validationCatalog) .as("Hadoop catalog does not support rename") .isNotInstanceOf(HadoopCatalog.class); From b82ca03a7cd4fb99ca866b8ecf6e3ceddf7a0003 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 10 Oct 2024 18:15:59 -0700 Subject: [PATCH 04/32] unneeded change --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 4add66c567bc..d6124ed693ec 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -178,10 +178,6 @@ public void before() { spark.conf().set("spark.sql.catalog." + catalogName + ".warehouse", "file:" + warehouse); } - if (!catalogName.equals("spark_catalog")) { - spark.conf().set("spark.sql.default.catalog", catalogName); - } - this.tableName = (catalogName.equals("spark_catalog") ? "" : catalogName + ".") + "default.table"; From 89a6a1d12525e17aa6d355cd349dc7e470042a86 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Fri, 11 Oct 2024 10:57:16 -0700 Subject: [PATCH 05/32] unneeded change --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index d6124ed693ec..a095db2a051d 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -93,17 +93,10 @@ private static void startRESTServer() { // prevent using already-in-use port when testing System.setProperty("rest.port", String.valueOf(MetaStoreUtils.findFreePort())); System.setProperty(CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath()); - // In-memory sqlite database by default is private to the connection that created it. - // If more than 1 jdbc connection backed by in-memory sqlite is created behind one - // JdbcCatalog, then different jdbc connections could provide different views of table - // status even belonging to the same catalog. Reference: - // https://www.sqlite.org/inmemorydb.html - System.setProperty(CatalogProperties.CLIENT_POOL_SIZE, "1"); restServer.start(false); restCatalog = RCKUtils.initCatalogClient(); System.clearProperty("rest.port"); System.clearProperty(CatalogProperties.WAREHOUSE_LOCATION); - System.clearProperty(CatalogProperties.CLIENT_POOL_SIZE); } catch (Exception e) { throw new RuntimeException(e); } From 00d9fcded036728c172348ed4bc95d4793238d69 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Fri, 11 Oct 2024 11:49:26 -0700 Subject: [PATCH 06/32] Revert "unneeded change" This reverts commit ae29c41769aefa0b2ae6f6102c4646337af5cad3. --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index a095db2a051d..d6124ed693ec 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -93,10 +93,17 @@ private static void startRESTServer() { // prevent using already-in-use port when testing System.setProperty("rest.port", String.valueOf(MetaStoreUtils.findFreePort())); System.setProperty(CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath()); + // In-memory sqlite database by default is private to the connection that created it. + // If more than 1 jdbc connection backed by in-memory sqlite is created behind one + // JdbcCatalog, then different jdbc connections could provide different views of table + // status even belonging to the same catalog. Reference: + // https://www.sqlite.org/inmemorydb.html + System.setProperty(CatalogProperties.CLIENT_POOL_SIZE, "1"); restServer.start(false); restCatalog = RCKUtils.initCatalogClient(); System.clearProperty("rest.port"); System.clearProperty(CatalogProperties.WAREHOUSE_LOCATION); + System.clearProperty(CatalogProperties.CLIENT_POOL_SIZE); } catch (Exception e) { throw new RuntimeException(e); } From 5010a717035385285fb941e14f880e03ce50f99f Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Fri, 11 Oct 2024 14:25:13 -0700 Subject: [PATCH 07/32] code format --- .../spark/extensions/TestMetadataTables.java | 9 +-- .../iceberg/spark/TestBaseWithCatalog.java | 57 ++++++++----------- .../iceberg/spark/sql/TestAlterTable.java | 8 +-- 3 files changed, 33 insertions(+), 41 deletions(-) diff --git a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java index 26f8e0d6ba08..cd623a9d6a42 100644 --- a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java +++ b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java @@ -22,7 +22,7 @@ import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_REST; import static org.apache.iceberg.types.Types.NestedField.optional; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assumptions.assumeFalse; +import static org.assertj.core.api.Assumptions.assumeThat; import java.io.IOException; import java.util.Comparator; @@ -743,9 +743,10 @@ private boolean partitionMatch(Record file, String partValue) { @TestTemplate public void metadataLogEntriesAfterReplacingTable() throws Exception { - // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on - // rest catalog - assumeFalse(ICEBERG_CATALOG_TYPE_REST.equals(catalogConfig.get(ICEBERG_CATALOG_TYPE))); + assumeThat(catalogConfig.get(ICEBERG_CATALOG_TYPE)) + .as( + "need to fix https://github.com/apache/iceberg/issues/11109 before enabling this for the REST catalog") + .isNotEqualTo(ICEBERG_CATALOG_TYPE_REST); sql( "CREATE TABLE %s (id bigint, data string) " diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index d6124ed693ec..0f5b821b49d8 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -26,7 +26,6 @@ import static org.assertj.core.api.Assertions.assertThat; import java.io.File; -import java.io.IOException; import java.util.Map; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -71,14 +70,14 @@ protected static Object[][] parameters() { } @BeforeAll - public static void setUpAll() throws IOException { + public static void setUpAll() throws Exception { TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null); assertThat(warehouse.delete()).isTrue(); startRESTServer(); } @AfterAll - public static void tearDownAll() throws IOException { + public static void tearDownAll() throws Exception { if (warehouse != null && warehouse.exists()) { Path warehousePath = new Path(warehouse.getAbsolutePath()); FileSystem fs = warehousePath.getFileSystem(hiveConf); @@ -87,38 +86,30 @@ public static void tearDownAll() throws IOException { stopRESTServer(); } - private static void startRESTServer() { - try { - restServer = new RESTCatalogServer(); - // prevent using already-in-use port when testing - System.setProperty("rest.port", String.valueOf(MetaStoreUtils.findFreePort())); - System.setProperty(CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath()); - // In-memory sqlite database by default is private to the connection that created it. - // If more than 1 jdbc connection backed by in-memory sqlite is created behind one - // JdbcCatalog, then different jdbc connections could provide different views of table - // status even belonging to the same catalog. Reference: - // https://www.sqlite.org/inmemorydb.html - System.setProperty(CatalogProperties.CLIENT_POOL_SIZE, "1"); - restServer.start(false); - restCatalog = RCKUtils.initCatalogClient(); - System.clearProperty("rest.port"); - System.clearProperty(CatalogProperties.WAREHOUSE_LOCATION); - System.clearProperty(CatalogProperties.CLIENT_POOL_SIZE); - } catch (Exception e) { - throw new RuntimeException(e); - } + private static void startRESTServer() throws Exception { + restServer = new RESTCatalogServer(); + // prevent using already-in-use port when testing + System.setProperty("rest.port", String.valueOf(MetaStoreUtils.findFreePort())); + System.setProperty(CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath()); + // In-memory sqlite database by default is private to the connection that created it. + // If more than 1 jdbc connection backed by in-memory sqlite is created behind one + // JdbcCatalog, then different jdbc connections could provide different views of table + // status even belonging to the same catalog. Reference: + // https://www.sqlite.org/inmemorydb.html + System.setProperty(CatalogProperties.CLIENT_POOL_SIZE, "1"); + restServer.start(false); + restCatalog = RCKUtils.initCatalogClient(); + System.clearProperty("rest.port"); + System.clearProperty(CatalogProperties.WAREHOUSE_LOCATION); + System.clearProperty(CatalogProperties.CLIENT_POOL_SIZE); } - private static void stopRESTServer() { - try { - if (restCatalog != null) { - restCatalog.close(); - } - if (restServer != null) { - restServer.stop(); - } - } catch (Exception e) { - throw new RuntimeException(e); + private static void stopRESTServer() throws Exception { + if (restCatalog != null) { + restCatalog.close(); + } + if (restServer != null) { + restServer.stop(); } } diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java index 0665427f9e8b..5abc72606f9f 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java @@ -23,7 +23,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assumptions.assumeThat; -import static org.junit.jupiter.api.Assumptions.assumeFalse; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; @@ -278,9 +277,10 @@ public void testAlterColumnPositionFirst() { @TestTemplate public void testTableRename() { - // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on - // rest catalog - assumeFalse(ICEBERG_CATALOG_TYPE_REST.equals(catalogConfig.get(ICEBERG_CATALOG_TYPE))); + assumeThat(catalogConfig.get(ICEBERG_CATALOG_TYPE)) + .as( + "need to fix https://github.com/apache/iceberg/issues/11154 before enabling this for the REST catalog") + .isNotEqualTo(ICEBERG_CATALOG_TYPE_REST); assumeThat(validationCatalog) .as("Hadoop catalog does not support rename") .isNotInstanceOf(HadoopCatalog.class); From 8ec19294686e82a883337082fda8d649ed0c764d Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Mon, 14 Oct 2024 13:41:42 -0700 Subject: [PATCH 08/32] Use in-mem config to configure RCK --- .../org/apache/iceberg/rest/RCKUtils.java | 6 ++++- .../iceberg/rest/RESTCatalogServer.java | 13 +++++++--- .../iceberg/spark/TestBaseWithCatalog.java | 26 +++++++++---------- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java index 1c2a265137b6..b1e6e57eac1d 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java @@ -77,7 +77,11 @@ static Map environmentCatalogConfig() { } public static RESTCatalog initCatalogClient() { - Map catalogProperties = Maps.newHashMap(); + return initCatalogClient(Maps.newHashMap()); + } + + public static RESTCatalog initCatalogClient(Map properties) { + Map catalogProperties = Maps.newHashMap(properties); catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); catalogProperties.putAll(Maps.fromProperties(System.getProperties())); diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java index 31ea1b696a3b..038b63cc3fab 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java @@ -38,12 +38,19 @@ public class RESTCatalogServer { private static final Logger LOG = LoggerFactory.getLogger(RESTCatalogServer.class); - static final String REST_PORT = "rest.port"; + public static final String REST_PORT = "rest.port"; static final int REST_PORT_DEFAULT = 8181; private Server httpServer; + private final Map config; - public RESTCatalogServer() {} + public RESTCatalogServer() { + this.config = Maps.newHashMap(); + } + + public RESTCatalogServer(Map config) { + this.config = config; + } static class CatalogContext { private final Catalog catalog; @@ -65,7 +72,7 @@ public Map configuration() { private CatalogContext initializeBackendCatalog() throws IOException { // Translate environment variables to catalog properties - Map catalogProperties = Maps.newHashMap(); + Map catalogProperties = Maps.newHashMap(config); catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); catalogProperties.putAll(Maps.fromProperties(System.getProperties())); diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 0f5b821b49d8..5bc6ccf8bcf3 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -87,21 +87,19 @@ public static void tearDownAll() throws Exception { } private static void startRESTServer() throws Exception { - restServer = new RESTCatalogServer(); - // prevent using already-in-use port when testing - System.setProperty("rest.port", String.valueOf(MetaStoreUtils.findFreePort())); - System.setProperty(CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath()); - // In-memory sqlite database by default is private to the connection that created it. - // If more than 1 jdbc connection backed by in-memory sqlite is created behind one - // JdbcCatalog, then different jdbc connections could provide different views of table - // status even belonging to the same catalog. Reference: - // https://www.sqlite.org/inmemorydb.html - System.setProperty(CatalogProperties.CLIENT_POOL_SIZE, "1"); + Map config = + Map.of( + RESTCatalogServer.REST_PORT, String.valueOf(MetaStoreUtils.findFreePort()), + CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), + // In-memory sqlite database by default is private to the connection that created it. + // If more than 1 jdbc connection backed by in-memory sqlite is created behind one + // JdbcCatalog, then different jdbc connections could provide different views of table + // status even belonging to the same catalog. Reference: + // https://www.sqlite.org/inmemorydb.html + CatalogProperties.CLIENT_POOL_SIZE, "1"); + restServer = new RESTCatalogServer(config); restServer.start(false); - restCatalog = RCKUtils.initCatalogClient(); - System.clearProperty("rest.port"); - System.clearProperty(CatalogProperties.WAREHOUSE_LOCATION); - System.clearProperty(CatalogProperties.CLIENT_POOL_SIZE); + restCatalog = RCKUtils.initCatalogClient(config); } private static void stopRESTServer() throws Exception { From c07b005a079d09de64056fea59438374c11adece Mon Sep 17 00:00:00 2001 From: Eduard Tudenhoefner Date: Wed, 23 Oct 2024 08:37:22 +0200 Subject: [PATCH 09/32] Update open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java --- .../java/org/apache/iceberg/rest/RESTCatalogServer.java | 1 - 1 file changed, 1 deletion(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java index 038b63cc3fab..a1b234eb49a2 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java @@ -74,7 +74,6 @@ private CatalogContext initializeBackendCatalog() throws IOException { // Translate environment variables to catalog properties Map catalogProperties = Maps.newHashMap(config); catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); - catalogProperties.putAll(Maps.fromProperties(System.getProperties())); // Fallback to a JDBCCatalog impl if one is not set catalogProperties.putIfAbsent(CatalogProperties.CATALOG_IMPL, JdbcCatalog.class.getName()); From 0a613f6dd2c9faadfd33a528013e2b3ff7c03743 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Tue, 29 Oct 2024 10:45:10 -0700 Subject: [PATCH 10/32] Use RESTServerExtension --- .../iceberg/rest/RESTServerExtension.java | 13 +++- .../iceberg/spark/TestBaseWithCatalog.java | 65 ++++++++++++------- 2 files changed, 53 insertions(+), 25 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java index fc3648055694..ae184812d34f 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java @@ -18,18 +18,29 @@ */ package org.apache.iceberg.rest; +import java.util.Map; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.junit.jupiter.api.extension.AfterAllCallback; import org.junit.jupiter.api.extension.BeforeAllCallback; import org.junit.jupiter.api.extension.ExtensionContext; public class RESTServerExtension implements BeforeAllCallback, AfterAllCallback { private RESTCatalogServer localServer; + private final Map config; + + public RESTServerExtension() { + config = Maps.newHashMap(); + } + + public RESTServerExtension(Map config) { + this.config = config; + } @Override public void beforeAll(ExtensionContext extensionContext) throws Exception { if (Boolean.parseBoolean( extensionContext.getConfigurationParameter(RCKUtils.RCK_LOCAL).orElse("true"))) { - this.localServer = new RESTCatalogServer(); + this.localServer = new RESTCatalogServer(config); this.localServer.start(false); } } diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 5bc6ccf8bcf3..39b775562d62 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -26,6 +26,7 @@ import static org.assertj.core.api.Assertions.assertThat; import java.io.File; +import java.io.IOException; import java.util.Map; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -45,17 +46,49 @@ import org.apache.iceberg.rest.RCKUtils; import org.apache.iceberg.rest.RESTCatalog; import org.apache.iceberg.rest.RESTCatalogServer; +import org.apache.iceberg.rest.RESTServerExtension; import org.apache.iceberg.util.PropertyUtil; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.api.io.TempDir; -@ExtendWith(ParameterizedTestExtension.class) +@ExtendWith({ParameterizedTestExtension.class, RESTServerExtension.class}) public abstract class TestBaseWithCatalog extends TestBase { - protected static File warehouse = null; - protected static RESTCatalogServer restServer; + protected static File warehouse; + + static { + try { + warehouse = File.createTempFile("warehouse", null); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private static final Map config; + + static { + try { + config = + Map.of( + RESTCatalogServer.REST_PORT, String.valueOf(MetaStoreUtils.findFreePort()), + CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), + // In-memory sqlite database by default is private to the connection that created it. + // If more than 1 jdbc connection backed by in-memory sqlite is created behind one + // JdbcCatalog, then different jdbc connections could provide different views of table + // status even belonging to the same catalog. Reference: + // https://www.sqlite.org/inmemorydb.html + CatalogProperties.CLIENT_POOL_SIZE, "1"); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @RegisterExtension + private static RESTServerExtension restServerExtension = new RESTServerExtension(config); + protected static RESTCatalog restCatalog; @Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}") @@ -70,10 +103,9 @@ protected static Object[][] parameters() { } @BeforeAll - public static void setUpAll() throws Exception { - TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null); + public static void setUpAll() { assertThat(warehouse.delete()).isTrue(); - startRESTServer(); + initRESTCatalog(); } @AfterAll @@ -83,32 +115,17 @@ public static void tearDownAll() throws Exception { FileSystem fs = warehousePath.getFileSystem(hiveConf); assertThat(fs.delete(warehousePath, true)).as("Failed to delete " + warehousePath).isTrue(); } - stopRESTServer(); + stopRESTCatalog(); } - private static void startRESTServer() throws Exception { - Map config = - Map.of( - RESTCatalogServer.REST_PORT, String.valueOf(MetaStoreUtils.findFreePort()), - CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), - // In-memory sqlite database by default is private to the connection that created it. - // If more than 1 jdbc connection backed by in-memory sqlite is created behind one - // JdbcCatalog, then different jdbc connections could provide different views of table - // status even belonging to the same catalog. Reference: - // https://www.sqlite.org/inmemorydb.html - CatalogProperties.CLIENT_POOL_SIZE, "1"); - restServer = new RESTCatalogServer(config); - restServer.start(false); + private static void initRESTCatalog() { restCatalog = RCKUtils.initCatalogClient(config); } - private static void stopRESTServer() throws Exception { + private static void stopRESTCatalog() throws Exception { if (restCatalog != null) { restCatalog.close(); } - if (restServer != null) { - restServer.stop(); - } } @TempDir protected java.nio.file.Path temp; From c914475323d6c943d85ad37e7f2199fc46090246 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Tue, 29 Oct 2024 11:55:17 -0700 Subject: [PATCH 11/32] check style and test failure --- .../apache/iceberg/spark/TestBaseWithCatalog.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 39b775562d62..865063350ede 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -67,11 +67,11 @@ public abstract class TestBaseWithCatalog extends TestBase { } } - private static final Map config; + private static final Map CONFIG; static { try { - config = + CONFIG = Map.of( RESTCatalogServer.REST_PORT, String.valueOf(MetaStoreUtils.findFreePort()), CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), @@ -87,7 +87,7 @@ public abstract class TestBaseWithCatalog extends TestBase { } @RegisterExtension - private static RESTServerExtension restServerExtension = new RESTServerExtension(config); + private static RESTServerExtension restServerExtension = new RESTServerExtension(CONFIG); protected static RESTCatalog restCatalog; @@ -103,7 +103,10 @@ protected static Object[][] parameters() { } @BeforeAll - public static void setUpAll() { + public static void setUpAll() throws IOException { + if (!warehouse.delete()) { + warehouse = File.createTempFile("warehouse", null); + } assertThat(warehouse.delete()).isTrue(); initRESTCatalog(); } @@ -119,7 +122,7 @@ public static void tearDownAll() throws Exception { } private static void initRESTCatalog() { - restCatalog = RCKUtils.initCatalogClient(config); + restCatalog = RCKUtils.initCatalogClient(CONFIG); } private static void stopRESTCatalog() throws Exception { From d18c3faf5be2de69128a77b2e2af6460c6b93efc Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Tue, 29 Oct 2024 11:59:14 -0700 Subject: [PATCH 12/32] test failure --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 865063350ede..8a8dd425532d 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -62,6 +62,7 @@ public abstract class TestBaseWithCatalog extends TestBase { static { try { warehouse = File.createTempFile("warehouse", null); + assertThat(warehouse.delete()).isTrue(); } catch (IOException e) { throw new RuntimeException(e); } @@ -104,9 +105,7 @@ protected static Object[][] parameters() { @BeforeAll public static void setUpAll() throws IOException { - if (!warehouse.delete()) { - warehouse = File.createTempFile("warehouse", null); - } + warehouse = File.createTempFile("warehouse", null); assertThat(warehouse.delete()).isTrue(); initRESTCatalog(); } From 82cca8b7ce634d5a21f11d962ca17f8966aa14bc Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Tue, 29 Oct 2024 13:02:21 -0700 Subject: [PATCH 13/32] fix test --- .../test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 8a8dd425532d..9712a5bfa85d 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -55,7 +55,7 @@ import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.api.io.TempDir; -@ExtendWith({ParameterizedTestExtension.class, RESTServerExtension.class}) +@ExtendWith(ParameterizedTestExtension.class) public abstract class TestBaseWithCatalog extends TestBase { protected static File warehouse; From 4672381b0dc861bb10e3894faf46c04e78ec8afd Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Tue, 29 Oct 2024 13:59:07 -0700 Subject: [PATCH 14/32] fix test --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 9712a5bfa85d..b61ed7cb1be8 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -105,8 +105,8 @@ protected static Object[][] parameters() { @BeforeAll public static void setUpAll() throws IOException { - warehouse = File.createTempFile("warehouse", null); - assertThat(warehouse.delete()).isTrue(); + // warehouse = File.createTempFile("warehouse", null); + // assertThat(warehouse.delete()).isTrue(); initRESTCatalog(); } From c1db1f9b93a4784307ad622a87deb669e86d54b9 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Tue, 29 Oct 2024 14:51:40 -0700 Subject: [PATCH 15/32] spotless --- .../testFixtures/java/org/apache/iceberg/rest/RCKUtils.java | 3 ++- .../java/org/apache/iceberg/rest/RESTCatalogServer.java | 4 ++-- .../spark/extensions/TestRemoveOrphanFilesProcedure.java | 2 +- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 4 +--- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java index b1e6e57eac1d..657d7aa46a64 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java @@ -81,9 +81,10 @@ public static RESTCatalog initCatalogClient() { } public static RESTCatalog initCatalogClient(Map properties) { - Map catalogProperties = Maps.newHashMap(properties); + Map catalogProperties = Maps.newHashMap(); catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); catalogProperties.putAll(Maps.fromProperties(System.getProperties())); + catalogProperties.putAll(properties); // Set defaults String port = diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java index a1b234eb49a2..6334b2f80162 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java @@ -72,8 +72,8 @@ public Map configuration() { private CatalogContext initializeBackendCatalog() throws IOException { // Translate environment variables to catalog properties - Map catalogProperties = Maps.newHashMap(config); - catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); + Map catalogProperties = Maps.newHashMap(RCKUtils.environmentCatalogConfig()); + catalogProperties.putAll(config); // Fallback to a JDBCCatalog impl if one is not set catalogProperties.putIfAbsent(CatalogProperties.CATALOG_IMPL, JdbcCatalog.class.getName()); diff --git a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java index 7731c613a6ba..399cd791a965 100644 --- a/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java +++ b/spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java @@ -452,7 +452,7 @@ public void testRemoveOrphanFilesWithStatisticFiles() throws Exception { String statsFileName = "stats-file-" + UUID.randomUUID(); String location = table.location(); // not every catalog will return file proto for local directories - // i.e. Hadoop and Hive Catalog does, Jdbc and REST does not + // i.e. Hadoop and Hive Catalog do, Jdbc and REST do not if (!location.startsWith("file:")) { location = "file:" + location; } diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index b61ed7cb1be8..a92c45bfc1fb 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -104,9 +104,7 @@ protected static Object[][] parameters() { } @BeforeAll - public static void setUpAll() throws IOException { - // warehouse = File.createTempFile("warehouse", null); - // assertThat(warehouse.delete()).isTrue(); + public static void setUpAll() { initRESTCatalog(); } From de4be14abb05ced0438f61573a88b6b8c46778b7 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Wed, 30 Oct 2024 10:52:53 -0700 Subject: [PATCH 16/32] Update open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java Co-authored-by: Eduard Tudenhoefner --- .../java/org/apache/iceberg/rest/RESTCatalogServer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java index 6334b2f80162..97ddad333228 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java @@ -44,7 +44,7 @@ public class RESTCatalogServer { private Server httpServer; private final Map config; - public RESTCatalogServer() { + RESTCatalogServer() { this.config = Maps.newHashMap(); } From 4e2dd03b948dfd92e71b5d8d359236f20bbfc0a8 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Wed, 30 Oct 2024 10:53:02 -0700 Subject: [PATCH 17/32] Update open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java Co-authored-by: Eduard Tudenhoefner --- .../java/org/apache/iceberg/rest/RESTCatalogServer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java index 97ddad333228..5f88f7b6666a 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java @@ -48,7 +48,7 @@ public class RESTCatalogServer { this.config = Maps.newHashMap(); } - public RESTCatalogServer(Map config) { + RESTCatalogServer(Map config) { this.config = config; } From c549eb357813704efabeb3ddf290178e95a4fa97 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Wed, 30 Oct 2024 10:54:20 -0700 Subject: [PATCH 18/32] Update spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Co-authored-by: Eduard Tudenhoefner --- .../test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index a92c45bfc1fb..ee0be8552343 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -119,7 +119,7 @@ public static void tearDownAll() throws Exception { } private static void initRESTCatalog() { - restCatalog = RCKUtils.initCatalogClient(CONFIG); + restCatalog = RCKUtils.initCatalogClient(restServerExtension.config()); } private static void stopRESTCatalog() throws Exception { From 043cd83d477e4a47e0ac60ac5fad65b7af37a447 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Wed, 30 Oct 2024 10:58:22 -0700 Subject: [PATCH 19/32] Spotless and fix test --- .../org/apache/iceberg/rest/RCKUtils.java | 16 +++++++++++++++ .../iceberg/rest/RESTCatalogServer.java | 4 ++-- .../iceberg/rest/RESTServerExtension.java | 4 ++++ .../iceberg/spark/TestBaseWithCatalog.java | 20 +++++-------------- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java index 657d7aa46a64..72657ea2cb08 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java @@ -18,6 +18,8 @@ */ package org.apache.iceberg.rest; +import java.io.IOException; +import java.net.ServerSocket; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -114,4 +116,18 @@ static void purgeCatalogTestEntries(RESTCatalog catalog) { catalog.dropNamespace(namespace); }); } + + public static int findFreePort() { + ServerSocket socket; + int port; + try { + socket = new ServerSocket(0); + port = socket.getLocalPort(); + socket.close(); + } catch (IOException e) { + throw new RuntimeException(e); + } + + return port; + } } diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java index 5f88f7b6666a..e79a590127fd 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java @@ -44,11 +44,11 @@ public class RESTCatalogServer { private Server httpServer; private final Map config; - RESTCatalogServer() { + RESTCatalogServer() { this.config = Maps.newHashMap(); } - RESTCatalogServer(Map config) { + RESTCatalogServer(Map config) { this.config = config; } diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java index ae184812d34f..03c5fc44883b 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java @@ -36,6 +36,10 @@ public RESTServerExtension(Map config) { this.config = config; } + public Map config() { + return config; + } + @Override public void beforeAll(ExtensionContext extensionContext) throws Exception { if (Boolean.parseBoolean( diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index ee0be8552343..6640c06d04b8 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -30,7 +30,6 @@ import java.util.Map; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; -import org.apache.hadoop.hive.metastore.MetaStoreUtils; import org.apache.iceberg.CatalogProperties; import org.apache.iceberg.Parameter; import org.apache.iceberg.ParameterizedTestExtension; @@ -68,27 +67,18 @@ public abstract class TestBaseWithCatalog extends TestBase { } } - private static final Map CONFIG; - - static { - try { - CONFIG = + @RegisterExtension + private static RESTServerExtension restServerExtension = + new RESTServerExtension( Map.of( - RESTCatalogServer.REST_PORT, String.valueOf(MetaStoreUtils.findFreePort()), + RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()), CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table // status even belonging to the same catalog. Reference: // https://www.sqlite.org/inmemorydb.html - CatalogProperties.CLIENT_POOL_SIZE, "1"); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - - @RegisterExtension - private static RESTServerExtension restServerExtension = new RESTServerExtension(CONFIG); + CatalogProperties.CLIENT_POOL_SIZE, "1")); protected static RESTCatalog restCatalog; From b854759e1837dfac44dd192dfacea4de376de3db Mon Sep 17 00:00:00 2001 From: Eduard Tudenhoefner Date: Thu, 31 Oct 2024 06:42:18 +0100 Subject: [PATCH 20/32] Apply suggestions from code review --- .../iceberg/spark/TestBaseWithCatalog.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 6640c06d04b8..8ccda18d0d70 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -56,23 +56,14 @@ @ExtendWith(ParameterizedTestExtension.class) public abstract class TestBaseWithCatalog extends TestBase { - protected static File warehouse; - - static { - try { - warehouse = File.createTempFile("warehouse", null); - assertThat(warehouse.delete()).isTrue(); - } catch (IOException e) { - throw new RuntimeException(e); - } - } + protected static File warehouse = null; + @RegisterExtension private static RESTServerExtension restServerExtension = new RESTServerExtension( Map.of( RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()), - CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table @@ -94,7 +85,9 @@ protected static Object[][] parameters() { } @BeforeAll - public static void setUpAll() { + public static void setUpAll() throws IOException { + TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null); + assertThat(warehouse.delete()).isTrue(); initRESTCatalog(); } From bf0dd18ffe202730a0f6d664a6fb8b840e645b79 Mon Sep 17 00:00:00 2001 From: Eduard Tudenhoefner Date: Thu, 31 Oct 2024 06:42:47 +0100 Subject: [PATCH 21/32] Apply suggestions from code review --- .../test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 1 - 1 file changed, 1 deletion(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 8ccda18d0d70..39ad9366e489 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -58,7 +58,6 @@ public abstract class TestBaseWithCatalog extends TestBase { protected static File warehouse = null; - @RegisterExtension private static RESTServerExtension restServerExtension = new RESTServerExtension( From b729f4f29a20ab6142002efdb13471478a67276b Mon Sep 17 00:00:00 2001 From: Eduard Tudenhoefner Date: Thu, 31 Oct 2024 06:47:54 +0100 Subject: [PATCH 22/32] Apply suggestions from code review --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 39ad9366e489..24202e450fb2 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -59,7 +59,7 @@ public abstract class TestBaseWithCatalog extends TestBase { protected static File warehouse = null; @RegisterExtension - private static RESTServerExtension restServerExtension = + private static final RESTServerExtension REST_SERVER_EXTENSION = new RESTServerExtension( Map.of( RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()), @@ -101,7 +101,7 @@ public static void tearDownAll() throws Exception { } private static void initRESTCatalog() { - restCatalog = RCKUtils.initCatalogClient(restServerExtension.config()); + restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config()); } private static void stopRESTCatalog() throws Exception { From 45b76f4675777d4375bf8a027d04f9a9f3ca9488 Mon Sep 17 00:00:00 2001 From: Eduard Tudenhoefner Date: Thu, 31 Oct 2024 06:55:11 +0100 Subject: [PATCH 23/32] Update spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 24202e450fb2..ad9d84869063 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -62,13 +62,15 @@ public abstract class TestBaseWithCatalog extends TestBase { private static final RESTServerExtension REST_SERVER_EXTENSION = new RESTServerExtension( Map.of( - RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()), + RESTCatalogServer.REST_PORT, + String.valueOf(RCKUtils.findFreePort()), // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table // status even belonging to the same catalog. Reference: // https://www.sqlite.org/inmemorydb.html - CatalogProperties.CLIENT_POOL_SIZE, "1")); + CatalogProperties.CLIENT_POOL_SIZE, + "1")); protected static RESTCatalog restCatalog; From c9f06b9e4959e89bb52c727051008fce9f2f52c0 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 31 Oct 2024 11:45:16 -0700 Subject: [PATCH 24/32] Package protected RCKUtils --- .../org/apache/iceberg/rest/RCKUtils.java | 22 ++--------- .../iceberg/rest/RESTServerExtension.java | 9 +++++ .../iceberg/spark/TestBaseWithCatalog.java | 37 +++++++++++-------- 3 files changed, 33 insertions(+), 35 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java index 72657ea2cb08..6787fbf73366 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java @@ -18,8 +18,6 @@ */ package org.apache.iceberg.rest; -import java.io.IOException; -import java.net.ServerSocket; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -31,7 +29,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.util.PropertyUtil; -public class RCKUtils { +class RCKUtils { private static final String CATALOG_ENV_PREFIX = "CATALOG_"; static final String RCK_LOCAL = "rck.local"; static final String RCK_PURGE_TEST_NAMESPACES = "rck.purge-test-namespaces"; @@ -78,11 +76,11 @@ static Map environmentCatalogConfig() { HashMap::new)); } - public static RESTCatalog initCatalogClient() { + static RESTCatalog initCatalogClient() { return initCatalogClient(Maps.newHashMap()); } - public static RESTCatalog initCatalogClient(Map properties) { + static RESTCatalog initCatalogClient(Map properties) { Map catalogProperties = Maps.newHashMap(); catalogProperties.putAll(RCKUtils.environmentCatalogConfig()); catalogProperties.putAll(Maps.fromProperties(System.getProperties())); @@ -116,18 +114,4 @@ static void purgeCatalogTestEntries(RESTCatalog catalog) { catalog.dropNamespace(namespace); }); } - - public static int findFreePort() { - ServerSocket socket; - int port; - try { - socket = new ServerSocket(0); - port = socket.getLocalPort(); - socket.close(); - } catch (IOException e) { - throw new RuntimeException(e); - } - - return port; - } } diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java index 03c5fc44883b..2f678f00a67e 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java @@ -26,6 +26,7 @@ public class RESTServerExtension implements BeforeAllCallback, AfterAllCallback { private RESTCatalogServer localServer; + private RESTCatalog client; private final Map config; public RESTServerExtension() { @@ -40,12 +41,17 @@ public Map config() { return config; } + public RESTCatalog client() { + return client; + } + @Override public void beforeAll(ExtensionContext extensionContext) throws Exception { if (Boolean.parseBoolean( extensionContext.getConfigurationParameter(RCKUtils.RCK_LOCAL).orElse("true"))) { this.localServer = new RESTCatalogServer(config); this.localServer.start(false); + this.client = RCKUtils.initCatalogClient(config); } } @@ -54,5 +60,8 @@ public void afterAll(ExtensionContext extensionContext) throws Exception { if (localServer != null) { localServer.stop(); } + if (client != null) { + client.close(); + } } } diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index ad9d84869063..fcbd73739964 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -27,6 +27,8 @@ import java.io.File; import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.ServerSocket; import java.util.Map; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -42,7 +44,6 @@ import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.hadoop.HadoopCatalog; import org.apache.iceberg.inmemory.InMemoryCatalog; -import org.apache.iceberg.rest.RCKUtils; import org.apache.iceberg.rest.RESTCatalog; import org.apache.iceberg.rest.RESTCatalogServer; import org.apache.iceberg.rest.RESTServerExtension; @@ -56,14 +57,23 @@ @ExtendWith(ParameterizedTestExtension.class) public abstract class TestBaseWithCatalog extends TestBase { - protected static File warehouse = null; + protected static File warehouse; + + static { + try { + warehouse = File.createTempFile("warehouse", null); + assertThat(warehouse.delete()).isTrue(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } @RegisterExtension private static final RESTServerExtension REST_SERVER_EXTENSION = new RESTServerExtension( Map.of( - RESTCatalogServer.REST_PORT, - String.valueOf(RCKUtils.findFreePort()), + RESTCatalogServer.REST_PORT, String.valueOf(findFreePort()), + CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table @@ -86,10 +96,8 @@ protected static Object[][] parameters() { } @BeforeAll - public static void setUpAll() throws IOException { - TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null); - assertThat(warehouse.delete()).isTrue(); - initRESTCatalog(); + public static void setUpAll() { + restCatalog = REST_SERVER_EXTENSION.client(); } @AfterAll @@ -99,16 +107,13 @@ public static void tearDownAll() throws Exception { FileSystem fs = warehousePath.getFileSystem(hiveConf); assertThat(fs.delete(warehousePath, true)).as("Failed to delete " + warehousePath).isTrue(); } - stopRESTCatalog(); - } - - private static void initRESTCatalog() { - restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config()); } - private static void stopRESTCatalog() throws Exception { - if (restCatalog != null) { - restCatalog.close(); + static int findFreePort() { + try (ServerSocket socket = new ServerSocket(0)) { + return socket.getLocalPort(); + } catch (IOException e) { + throw new UncheckedIOException(e); } } From 2f1ad6c05fa1cfacf038dcd58e707e809401db9a Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 31 Oct 2024 11:49:35 -0700 Subject: [PATCH 25/32] spotless --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index fcbd73739964..8ec1fd405e84 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -72,8 +72,10 @@ public abstract class TestBaseWithCatalog extends TestBase { private static final RESTServerExtension REST_SERVER_EXTENSION = new RESTServerExtension( Map.of( - RESTCatalogServer.REST_PORT, String.valueOf(findFreePort()), - CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(), + RESTCatalogServer.REST_PORT, + String.valueOf(findFreePort()), + CatalogProperties.WAREHOUSE_LOCATION, + warehouse.getAbsolutePath(), // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table From a23bc7f232dbd5746bd8433d999db7597d042710 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 31 Oct 2024 11:53:58 -0700 Subject: [PATCH 26/32] unintentional change --- .../test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 8ec1fd405e84..9244f9925b6d 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -103,7 +103,7 @@ public static void setUpAll() { } @AfterAll - public static void tearDownAll() throws Exception { + public static void tearDownAll() throws IOException { if (warehouse != null && warehouse.exists()) { Path warehousePath = new Path(warehouse.getAbsolutePath()); FileSystem fs = warehousePath.getFileSystem(hiveConf); From eb1a345cae2d219a01ef1edbf751904cb8ed092e Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 31 Oct 2024 11:57:27 -0700 Subject: [PATCH 27/32] remove warehouse specification from rest --- .../java/org/apache/iceberg/spark/TestBaseWithCatalog.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 9244f9925b6d..7ebad230b2f7 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -74,8 +74,8 @@ public abstract class TestBaseWithCatalog extends TestBase { Map.of( RESTCatalogServer.REST_PORT, String.valueOf(findFreePort()), - CatalogProperties.WAREHOUSE_LOCATION, - warehouse.getAbsolutePath(), + // CatalogProperties.WAREHOUSE_LOCATION, + // warehouse.getAbsolutePath(), // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table From 048f037d7e61bc1ed529ca2d918a9880779ae251 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 31 Oct 2024 13:07:04 -0700 Subject: [PATCH 28/32] spotless --- .../iceberg/spark/TestBaseWithCatalog.java | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index 7ebad230b2f7..a6722b839d24 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -57,16 +57,7 @@ @ExtendWith(ParameterizedTestExtension.class) public abstract class TestBaseWithCatalog extends TestBase { - protected static File warehouse; - - static { - try { - warehouse = File.createTempFile("warehouse", null); - assertThat(warehouse.delete()).isTrue(); - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } + protected static File warehouse = null; @RegisterExtension private static final RESTServerExtension REST_SERVER_EXTENSION = @@ -74,8 +65,6 @@ public abstract class TestBaseWithCatalog extends TestBase { Map.of( RESTCatalogServer.REST_PORT, String.valueOf(findFreePort()), - // CatalogProperties.WAREHOUSE_LOCATION, - // warehouse.getAbsolutePath(), // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table @@ -98,7 +87,9 @@ protected static Object[][] parameters() { } @BeforeAll - public static void setUpAll() { + public static void setUpAll() throws IOException { + TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null); + assertThat(warehouse.delete()).isTrue(); restCatalog = REST_SERVER_EXTENSION.client(); } From bac5d1758259f2dc7b9a6d4e7e221faafbcadd0b Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 14 Nov 2024 10:37:15 -0800 Subject: [PATCH 29/32] move find free port to rest server extension --- .../java/org/apache/iceberg/rest/RCKUtils.java | 11 +++++++++++ .../org/apache/iceberg/rest/RESTServerExtension.java | 10 ++++++++++ .../apache/iceberg/spark/TestBaseWithCatalog.java | 12 +----------- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java index 6787fbf73366..adeba4709329 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java @@ -18,6 +18,9 @@ */ package org.apache.iceberg.rest; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.ServerSocket; import java.util.HashMap; import java.util.List; import java.util.Locale; @@ -114,4 +117,12 @@ static void purgeCatalogTestEntries(RESTCatalog catalog) { catalog.dropNamespace(namespace); }); } + + static int findFreePort() { + try (ServerSocket socket = new ServerSocket(0)) { + return socket.getLocalPort(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } } diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java index 2f678f00a67e..b9aa7077f01e 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java @@ -18,6 +18,8 @@ */ package org.apache.iceberg.rest; +import static org.apache.iceberg.rest.RESTCatalogServer.REST_PORT; + import java.util.Map; import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.junit.jupiter.api.extension.AfterAllCallback; @@ -25,6 +27,10 @@ import org.junit.jupiter.api.extension.ExtensionContext; public class RESTServerExtension implements BeforeAllCallback, AfterAllCallback { + // if the caller explicitly wants the server to start on port 0, it means the caller wants to + // launch on a free port + public static final String FREE_PORT = "0"; + private RESTCatalogServer localServer; private RESTCatalog client; private final Map config; @@ -34,6 +40,10 @@ public RESTServerExtension() { } public RESTServerExtension(Map config) { + Map conf = Maps.newHashMap(config); + if (conf.containsKey(REST_PORT) && conf.get(REST_PORT).equals(FREE_PORT)) { + conf.put(REST_PORT, String.valueOf(RCKUtils.findFreePort())); + } this.config = config; } diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java index a6722b839d24..96dbd94a805d 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java @@ -27,8 +27,6 @@ import java.io.File; import java.io.IOException; -import java.io.UncheckedIOException; -import java.net.ServerSocket; import java.util.Map; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -64,7 +62,7 @@ public abstract class TestBaseWithCatalog extends TestBase { new RESTServerExtension( Map.of( RESTCatalogServer.REST_PORT, - String.valueOf(findFreePort()), + RESTServerExtension.FREE_PORT, // In-memory sqlite database by default is private to the connection that created it. // If more than 1 jdbc connection backed by in-memory sqlite is created behind one // JdbcCatalog, then different jdbc connections could provide different views of table @@ -102,14 +100,6 @@ public static void tearDownAll() throws IOException { } } - static int findFreePort() { - try (ServerSocket socket = new ServerSocket(0)) { - return socket.getLocalPort(); - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } - @TempDir protected java.nio.file.Path temp; @Parameter(index = 0) From 85655d8a5b73d63e85bc96ed38ff391ef2d86410 Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 14 Nov 2024 10:39:30 -0800 Subject: [PATCH 30/32] fix typo --- .../java/org/apache/iceberg/rest/RESTServerExtension.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java index b9aa7077f01e..318fbdc3a666 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java @@ -44,7 +44,7 @@ public RESTServerExtension(Map config) { if (conf.containsKey(REST_PORT) && conf.get(REST_PORT).equals(FREE_PORT)) { conf.put(REST_PORT, String.valueOf(RCKUtils.findFreePort())); } - this.config = config; + this.config = conf; } public Map config() { From 0ef2c2bc49affaaed8e38e78f4888a9e1b4b8f7c Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 14 Nov 2024 11:00:51 -0800 Subject: [PATCH 31/32] checkstyle --- .../java/org/apache/iceberg/rest/RESTServerExtension.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java index 318fbdc3a666..19236bec64c3 100644 --- a/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java +++ b/open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTServerExtension.java @@ -18,8 +18,6 @@ */ package org.apache.iceberg.rest; -import static org.apache.iceberg.rest.RESTCatalogServer.REST_PORT; - import java.util.Map; import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.junit.jupiter.api.extension.AfterAllCallback; @@ -41,8 +39,9 @@ public RESTServerExtension() { public RESTServerExtension(Map config) { Map conf = Maps.newHashMap(config); - if (conf.containsKey(REST_PORT) && conf.get(REST_PORT).equals(FREE_PORT)) { - conf.put(REST_PORT, String.valueOf(RCKUtils.findFreePort())); + if (conf.containsKey(RESTCatalogServer.REST_PORT) + && conf.get(RESTCatalogServer.REST_PORT).equals(FREE_PORT)) { + conf.put(RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort())); } this.config = conf; } From 2d31dc0bf43e7f32a0287c104dd82ed8baf6441d Mon Sep 17 00:00:00 2001 From: Haizhou Zhao Date: Thu, 14 Nov 2024 16:58:39 -0800 Subject: [PATCH 32/32] fix unit test --- .../java/org/apache/iceberg/spark/sql/TestCreateTable.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestCreateTable.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestCreateTable.java index 11d4cfebfea6..39aeacf68b9a 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestCreateTable.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestCreateTable.java @@ -18,6 +18,8 @@ */ package org.apache.iceberg.spark.sql; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE; +import static org.apache.iceberg.CatalogUtil.ICEBERG_CATALOG_TYPE_REST; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assumptions.assumeThat; @@ -351,6 +353,10 @@ public void testCreateTableProperties() { @TestTemplate public void testCreateTableCommitProperties() { + assumeThat(catalogConfig.get(ICEBERG_CATALOG_TYPE)) + .as( + "need to fix https://github.com/apache/iceberg/issues/11554 before enabling this for the REST catalog") + .isNotEqualTo(ICEBERG_CATALOG_TYPE_REST); assertThat(validationCatalog.tableExists(tableIdent)) .as("Table should not already exist") .isFalse();