You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "haizhou-zhao (via GitHub)" <gi...@apache.org> on 2024/09/06 21:18:01 UTC
[PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
haizhou-zhao opened a new pull request, #11093:
URL: https://github.com/apache/iceberg/pull/11093
For issue: https://github.com/apache/iceberg/issues/11079
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823794001
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,16 +41,46 @@
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.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)
public abstract class TestBaseWithCatalog extends TestBase {
- protected static File warehouse = null;
+ protected static File warehouse;
Review Comment:
```suggestion
protected static File warehouse = null;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1821577479
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +72,8 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap(config);
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797263955
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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");
Review Comment:
RCK interface right now only supports system properties. Let me see what we can do there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797439776
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -740,6 +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)));
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796534509
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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");
Review Comment:
maybe we should pass a Map<String, String> to the REST server rather than having to set system properties (which then also need to be cleared again)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797438855
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +70,47 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws Exception {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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() 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");
Review Comment:
There was this conversation getting lost (not sure why): https://github.com/apache/iceberg/pull/11093#discussion_r1791488648
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797438018
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java:
##########
@@ -275,6 +278,11 @@ public void testAlterColumnPositionFirst() {
@TestTemplate
public void testTableRename() {
+ if (Set.of(ICEBERG_CATALOG_TYPE_REST).contains(catalogConfig.get(ICEBERG_CATALOG_TYPE))) {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1822138079
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -18,13 +18,19 @@
*/
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;
import java.io.IOException;
import java.util.Map;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.metastore.MetaStoreUtils;
Review Comment:
I'm not sure whether we should be using this dependency from Hive here. Can we do it in a different way? Maybe add
```
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;
}
```
to RCKUtils?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1825086332
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,16 +41,46 @@
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.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)
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 RuntimeException(e);
+ }
+ }
+
+ @RegisterExtension
+ private static RESTServerExtension restServerExtension =
+ new RESTServerExtension(
+ Map.of(
+ RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()),
+ CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(),
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1841393266
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,20 +87,29 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ restCatalog = REST_SERVER_EXTENSION.client();
Review Comment:
I feel like we're almost there, but we're still doing too much here that should be one in the extension.
We should make this test base implement either the `BeforeAllCallback` or the `BeforeEachCallback` which allows for using the extension store:
```java
@Override
public void beforeAll(ExtensionContext extensionContext) {
restCatalog = (RESTCatalog) extensionContext.getStore(ExtensionContext.Namespace.create("rest")).get("rest-catalog");
}
```
This will actually get called before the `@BeforeAll` annotation, so we can capture what we need to from the context store and configure for the rest of the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796544204
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -89,13 +138,37 @@ public static void dropWarehouse() throws IOException {
protected TableIdentifier tableIdent = TableIdentifier.of(Namespace.of("default"), "table");
protected String tableName;
+ private void configureValidationCatalog() {
+ 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;
+ }
+
@BeforeEach
public void before() {
- this.validationCatalog =
Review Comment:
what happens if we don't do any changes to how the validation catalog is configured?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796545689
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +65,9 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap();
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
+ catalogProperties.putAll(Maps.fromProperties(System.getProperties()));
Review Comment:
as mentioned in a comment further above, maybe we should consider passing a Map<String, String> to `RESTCatalogServer` rather than relying on system properties (which have to be cleared after they were configured)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823794365
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,16 +41,46 @@
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.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)
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 RuntimeException(e);
+ }
+ }
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823795448
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,17 +41,38 @@
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.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)
public abstract class TestBaseWithCatalog extends TestBase {
protected static File warehouse = null;
+
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796534994
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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 {
Review Comment:
maybe just remove the try catch and just declare that this method is throwing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823145510
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,16 +42,55 @@
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.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)
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 RuntimeException(e);
+ }
+ }
+
+ private static final Map<String, String> 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);
Review Comment:
Done, config in-lined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1842636900
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,20 +87,29 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ restCatalog = REST_SERVER_EXTENSION.client();
Review Comment:
I think if you were to use the `BeforeEachCallback` interface, you could fully replace the static restCatalog reference and just pull from the context when the validating catalog is rest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1841393266
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,20 +87,29 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ restCatalog = REST_SERVER_EXTENSION.client();
Review Comment:
I feel like we're almost there, but we're still doing too much here that should be one in the extension.
We should make this test base implement either the `BeforeAllCallback` or the `BeforeEachCallback` which allows for using the extension store:
```java
@Override
public void beforeAll(ExtensionContext extensionContext) {
restCatalog = (RESTCatalog) extensionContext.getStore(ExtensionContext.Namespace.create("rest")).get("rest-catalog");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1822083568
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -37,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<String, String> config;
- RESTCatalogServer() {}
+ public RESTCatalogServer() {
+ this.config = Maps.newHashMap();
+ }
+
+ public RESTCatalogServer(Map<String, String> config) {
Review Comment:
```suggestion
RESTCatalogServer(Map<String, String> config) {
```
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -37,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<String, String> config;
- RESTCatalogServer() {}
+ public RESTCatalogServer() {
Review Comment:
```suggestion
RESTCatalogServer() {
```
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +104,28 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
- TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
- assertThat(warehouse.delete()).isTrue();
+ public static void setUpAll() {
+ initRESTCatalog();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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();
}
+ stopRESTCatalog();
+ }
+
+ private static void initRESTCatalog() {
+ restCatalog = RCKUtils.initCatalogClient(CONFIG);
Review Comment:
maybe just add
```
public Map<String, String> config() {
return config;
}
```
to the extension class, so that this can be changed to
```suggestion
restCatalog = RCKUtils.initCatalogClient(restServerExtension.config());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#issuecomment-2450262762
Hey @haizhou-zhao this is looking really close. Some minor comments on how I think we can improve the Extension handling to better isolate the test, but other than that, I think it looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823794984
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,16 +41,46 @@
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.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)
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 RuntimeException(e);
+ }
+ }
+
+ @RegisterExtension
+ private static RESTServerExtension restServerExtension =
+ new RESTServerExtension(
+ Map.of(
+ RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()),
+ CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(),
Review Comment:
I commented on this yesterday but can't find the comment anymore. The `warehouse` is currently only used for Hadoop and the REST server will create a separate temp directory for the REST catalog's warehouse, so we can remove this
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,16 +41,46 @@
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.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)
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 RuntimeException(e);
+ }
+ }
+
+ @RegisterExtension
+ private static RESTServerExtension restServerExtension =
+ new RESTServerExtension(
+ Map.of(
+ RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()),
+ CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(),
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796507653
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -18,8 +18,11 @@
*/
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 static org.junit.jupiter.api.Assumptions.assumeFalse;
Review Comment:
please use AssertJ assumptions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796527938
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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 stopRESTServer() {
Review Comment:
```suggestion
private static void stopRESTServer() throws Exception {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797263955
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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");
Review Comment:
RCK interface right now only supports system properties. Let me see what we can do there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1811965819
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +72,9 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap(config);
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
+ catalogProperties.putAll(Maps.fromProperties(System.getProperties()));
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1824749990
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java:
##########
@@ -76,15 +78,22 @@ static Map<String, String> environmentCatalogConfig() {
HashMap::new));
}
- static RESTCatalog initCatalogClient() {
+ public static RESTCatalog initCatalogClient() {
Review Comment:
I think we can get away with leaving this as package protected and not expose the inner workings of the RCK and Extension. Take a look at my example below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1824745679
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +86,30 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ initRESTCatalog();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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();
}
+ stopRESTCatalog();
+ }
+
+ private static void initRESTCatalog() {
+ restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config());
Review Comment:
So rather than making the client like theis and making the RESTUtils public. I think you can actually create the client in the extension and just grab it out of the context:
```java
// In the RESTServerExtension class
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
// ... other init
// store the client
Store store = context.getStore(Namespace.GLOBAL);
store.put("rest-client", RCKUtils.initCatalogClient(...));
}
}
```
In this class then just use
```java
@Override
public void beforeEach(ExtensionContext context) {
// get the rest catalog client from the context store
Store store = context.getStore(Namespace.GLOBAL);
this.restCatalog = store.get("rest-client");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1825101193
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +86,30 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ initRESTCatalog();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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();
}
+ stopRESTCatalog();
+ }
+
+ private static void initRESTCatalog() {
+ restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config());
Review Comment:
I made `RCKUtils` package protected again in the latest commit. A little bit different from what you suggested above is: the Spark integ test rely on REST client being initialized in static context, so initializing restCatalog in `beforeEach` might be hard. Solved the problem by making `RESTServerExtention` taking control over client life cycle along side server life cycle. Anyway, in the end, `RCKUtils` is package protected again.
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java:
##########
@@ -76,15 +78,22 @@ static Map<String, String> environmentCatalogConfig() {
HashMap::new));
}
- static RESTCatalog initCatalogClient() {
+ public static RESTCatalog initCatalogClient() {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1814315018
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +72,8 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap(config);
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
Review Comment:
I actually handled something else. So this still needs to be done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1821584310
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +65,9 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap();
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
+ catalogProperties.putAll(Maps.fromProperties(System.getProperties()));
Review Comment:
Reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823798127
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,17 +41,37 @@
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.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)
public abstract class TestBaseWithCatalog extends TestBase {
protected static File warehouse = null;
+ @RegisterExtension
+ private static RESTServerExtension restServerExtension =
Review Comment:
```suggestion
private static final RESTServerExtension REST_SERVER_EXTENSION =
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797427317
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -105,6 +166,10 @@ public void before() {
spark.conf().set("spark.sql.catalog." + catalogName + ".warehouse", "file:" + warehouse);
}
+ if (!catalogName.equals("spark_catalog")) {
Review Comment:
unneeded, removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1800101451
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +65,9 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap();
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
+ catalogProperties.putAll(Maps.fromProperties(System.getProperties()));
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797262749
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -89,13 +138,37 @@ public static void dropWarehouse() throws IOException {
protected TableIdentifier tableIdent = TableIdentifier.of(Namespace.of("default"), "table");
protected String tableName;
+ private void configureValidationCatalog() {
+ 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;
+ }
+
@BeforeEach
public void before() {
- this.validationCatalog =
Review Comment:
something like you created a table on REST catalog, while validate against Hive catalog.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797439934
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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 stopRESTServer() {
+ try {
+ if (restCatalog != null) {
+ restCatalog.close();
+ }
+ if (restServer != null) {
+ restServer.stop();
+ }
+ } catch (Exception e) {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823793865
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +94,28 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
- TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
- assertThat(warehouse.delete()).isTrue();
+ public static void setUpAll() {
+ initRESTCatalog();
Review Comment:
```suggestion
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
initRESTCatalog();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823793249
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +94,28 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
- TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
- assertThat(warehouse.delete()).isTrue();
+ public static void setUpAll() {
+ initRESTCatalog();
Review Comment:
```suggestion
warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
initRESTCatalog();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823797130
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,17 +41,37 @@
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.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)
public abstract class TestBaseWithCatalog extends TestBase {
protected static File warehouse = null;
+ @RegisterExtension
+ private static RESTServerExtension restServerExtension =
Review Comment:
I think we should now be able to make this `final`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1825097791
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java:
##########
@@ -107,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);
Review Comment:
Both done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1822131912
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -37,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<String, String> config;
- RESTCatalogServer() {}
+ public RESTCatalogServer() {
Review Comment:
I don't think this needs to be public anymore, so we can just go back to how it was before
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796516348
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java:
##########
@@ -275,6 +278,9 @@ public void testAlterColumnPositionFirst() {
@TestTemplate
public void testTableRename() {
+ // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on
Review Comment:
```
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797318587
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -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");
Review Comment:
Yes, this is needed for test to pass. I attempted to run test without this line and here're the result: https://github.com/apache/iceberg/actions/runs/11297060489/job/31423193928?pr=11093
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796527051
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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 stopRESTServer() {
+ try {
+ if (restCatalog != null) {
+ restCatalog.close();
+ }
+ if (restServer != null) {
+ restServer.stop();
+ }
+ } catch (Exception e) {
Review Comment:
I don't think we need the try catch here. I'd just declare the exception at the method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797441874
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +70,47 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws Exception {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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() 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");
Review Comment:
This one was also lost: https://github.com/apache/iceberg/pull/11093#discussion_r1796534509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1799088453
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -89,13 +138,37 @@ public static void dropWarehouse() throws IOException {
protected TableIdentifier tableIdent = TableIdentifier.of(Namespace.of("default"), "table");
protected String tableName;
+ private void configureValidationCatalog() {
+ 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;
+ }
+
@BeforeEach
public void before() {
- this.validationCatalog =
Review Comment:
my point is that I think all tests should be passing when we don't do any changes to the validation catalog
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1800167581
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +70,47 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws Exception {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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() 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");
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1822139250
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,16 +42,55 @@
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.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)
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 RuntimeException(e);
+ }
+ }
+
+ private static final Map<String, String> 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);
Review Comment:
I believe we should be able to in-line the config here after all the other comments have been applied
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1852643846
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,20 +87,29 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ restCatalog = REST_SERVER_EXTENSION.client();
Review Comment:
I'm thinking, the other way of refactoring this is we can initialize Spark session in a BeforeAllCallBack extension class (and that perhaps also mean we need to refactor the existing HiveCatalog & HadoopCatalog initialization into separate Extension classes to take care of the existing Spark tests).
Like mentioned before, as long as we are still initializing Spark session in BeforeAll stage, we unlikely can delay init of catalogs to BeforeEach stage.
Yet if the Spark session is also init in a separate extension class, we might not need to write getters on the extension class for catalogs to expose anything related to catalogs to the outside world (info can pass freely between extension classes) - we only need to write getters for Spark extension class so that the test classes themselves can get a handle to the Spark session.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1842726587
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,20 +87,29 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ restCatalog = REST_SERVER_EXTENSION.client();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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();
}
}
+ static int findFreePort() {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1811964069
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +65,9 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap();
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
+ catalogProperties.putAll(Maps.fromProperties(System.getProperties()));
Review Comment:
that means we can revert this line here: `catalogProperties.putAll(Maps.fromProperties(System.getProperties()));`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1813548551
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +72,8 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap(config);
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
Review Comment:
sounds good. I saw you already handled this through your suggested commit. I adopted your code change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823798343
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +84,30 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ initRESTCatalog();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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();
}
+ stopRESTCatalog();
+ }
+
+ private static void initRESTCatalog() {
+ restCatalog = RCKUtils.initCatalogClient(restServerExtension.config());
Review Comment:
```suggestion
restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1795890640
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -740,6 +743,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))) {
Review Comment:
thx, replacing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1795891887
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,47 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void createWarehouseAndStartRest() throws IOException {
Review Comment:
ack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796534509
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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");
Review Comment:
maybe we should pass a Map<String, String> to the REST server rather than having to set system properties (which then also need to be cleared again). @danielcweeks thoughts on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796516710
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -740,6 +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)));
Review Comment:
```suggestion
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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1795896887
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -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 {
Review Comment:
ack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797439446
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -18,8 +18,11 @@
*/
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 static org.junit.jupiter.api.Assumptions.assumeFalse;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #11093:
URL: https://github.com/apache/iceberg/pull/11093
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797440072
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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
+ 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 stopRESTServer() {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797439642
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java:
##########
@@ -275,6 +278,9 @@ public void testAlterColumnPositionFirst() {
@TestTemplate
public void testTableRename() {
+ // need to fix https://github.com/apache/iceberg/issues/11109 before enabling this test on
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#issuecomment-2490793742
@nastra, @danielcweeks is there something stopping us from merging this?
This is currently blocking us on https://github.com/apache/iceberg/pull/11317 which is a requirement for REST Catalogs to offer undrop for storages that are not S3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1796004376
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -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)) {
Review Comment:
ack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1824709276
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java:
##########
@@ -107,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);
Review Comment:
Nit: This could use try-with-resources and we should probably use UncheckedIOException rather than RE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823145272
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -18,13 +18,19 @@
*/
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;
import java.io.IOException;
import java.util.Map;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.metastore.MetaStoreUtils;
Review Comment:
Done, added the util above to RCKUtils and removed deps on hive lib.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823793249
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +94,28 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
- TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
- assertThat(warehouse.delete()).isTrue();
+ public static void setUpAll() {
+ initRESTCatalog();
Review Comment:
```suggestion
warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
initRESTCatalog();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1825102384
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java:
##########
@@ -107,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);
Review Comment:
Btw, since `RCKUtils` is being package protected again, and this findFreePort util is needed on the Spark integ side, I just moved this util out of `RCKUtils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1815793488
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +70,45 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws Exception {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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() throws Exception {
Review Comment:
Is there no way to use the `RESTServerExtention` provided by the test fixtures? I'd rather not expose the RCKUtils and manage the service lifecycle manually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1811968596
##########
open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java:
##########
@@ -64,7 +72,8 @@ public Map<String, String> configuration() {
private CatalogContext initializeBackendCatalog() throws IOException {
// Translate environment variables to catalog properties
- Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+ Map<String, String> catalogProperties = Maps.newHashMap(config);
+ catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
Review Comment:
I think we might want to turn the maps around, so that the initial map comes from `RCKUtils.environmentCatalogConfig()` and then we'd do `catalogProperties.putAll(config)`. That way we can override stuff via `config`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1797440932
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +71,55 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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 {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1795881502
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java:
##########
@@ -521,7 +524,7 @@ public void testFilesTableTimeTravelWithSchemaEvolution() throws Exception {
optional(3, "category", Types.StringType.get())));
spark.createDataFrame(newRecords, newSparkSchema).coalesce(1).writeTo(tableName).append();
-
+ table.refresh();
Review Comment:
Yes, RESTTableOperations and other TableOperations has different mechanisms of refreshing metadata.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1800188393
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -89,13 +138,37 @@ public static void dropWarehouse() throws IOException {
protected TableIdentifier tableIdent = TableIdentifier.of(Namespace.of("default"), "table");
protected String tableName;
+ private void configureValidationCatalog() {
+ 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;
+ }
+
@BeforeEach
public void before() {
- this.validationCatalog =
Review Comment:
If I understand correctly, the original `validationCatalog` is set to a `HadoopCatalog` if `catalogName` of the catalog being tested is named `testhadoop`; otherwise, make the `validationCatalog` the same as `catalog` (which is strictly a `HiveCatalog`, as defined by `TestBase` class).
That will work in the old days, as we only have 2 types catalogs, either Hadoop or Hive, being tested - if the test subject is not a Hadoop Catalog, then setting the validation catalog to Hive Catalog will suffice the validation purpose. However, with the introduction of a 3rd type, REST catalog: when the test subject is a REST catalog, without changing how `validationCatalog` is initialized, the `validationCatalog` will be set to a Hive catalog. In this case, conducting test behaviors on REST catalog while validating the status post-change on Hive catalog won't work.
Unless, you are suggesting that we should make changes to `TestBase` class where the `catalog` being tested does not strictly need to be a HiveCatalog, and can be any type of catalog.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1799089974
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +70,47 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws Exception {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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() 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");
Review Comment:
As I mentioned in https://github.com/apache/iceberg/pull/11093#discussion_r1796534509 I think we should do this differently rather than passing/re-setting system properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1821577268
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +70,45 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws Exception {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ startRESTServer();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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() throws Exception {
Review Comment:
Done, changed to use `RESTServerExtention`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1852627137
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,20 +87,29 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ restCatalog = REST_SERVER_EXTENSION.client();
Review Comment:
As discussed offline, the biggest issue right now to incorporate rest client init into BeforeEach call are the fact that
1. REST server&client (and all their configurations) are initialized in Extension, not in the test class itself
2. REST server (port) is dynamically assigned, but that information is needed to initialize Spark session in a static context
For 1, the current work around is to give the extension class a getter method so that the test class can obtain necessary information.
For 2, as an example, the existing test Hive Metastore server for spark tests also has its port dynamically assigned. The existing approach is that the test Hive Metastore will be initialized in the test class itself in its BeforeAll call, so that the server hiveConf could be used to initialize Spark session in later steps of the BeforeAll calls. Looking back at rest client, if we don't refactor the Spark session to be initialized in BeforeEach stage, then it's unlikely we can delay the init of rest client to BeforeEach stage. Meanwhile, init Spark session at BeforeEach stage (every test has its own Spark session spin up) would be quite expensive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823802445
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -36,17 +41,37 @@
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.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)
public abstract class TestBaseWithCatalog extends TestBase {
protected static File warehouse = null;
+ @RegisterExtension
+ private static final RESTServerExtension REST_SERVER_EXTENSION =
+ new RESTServerExtension(
+ Map.of(
+ 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"));
Review Comment:
```suggestion
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"));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1824745679
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +86,30 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ initRESTCatalog();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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();
}
+ stopRESTCatalog();
+ }
+
+ private static void initRESTCatalog() {
+ restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config());
Review Comment:
So rather than making the client like theis and making the RESTUtils public. I think you can actually create the client in the extension and just grab it out of the context:
```java
// In the RESTServerExtension class
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
// ... other init
// store the client
Store store = context.getStore(Namespace.GLOBAL);
store.put("rest-client", RCKUtils.initCatalogClient(...));
}
}
```
In this class then just use
```java
@Override
public void beforeEach(ExtensionContext context) {
// get the rest catalog client from the context store
Store store = context.getStore(Namespace.GLOBAL);
this.restCatalog = store.get("rest-client");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1824745679
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +86,30 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ initRESTCatalog();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ public static void tearDownAll() throws Exception {
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();
}
+ stopRESTCatalog();
+ }
+
+ private static void initRESTCatalog() {
+ restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config());
Review Comment:
So rather than making the client like theis and making the RESTUtils public. I think you can actually create the client in the extension and just grab it out of the context:
```java
// In the RESTServerExtension class
@Override
public void beforeAll(ExtensionContext extensionContext) throws Exception {
// ... other init
// store the client
Store store = context.getStore(Namespace.GLOBAL);
store.put("rest-client", RCKUtils.initCatalogClient(...));
}
}
```
```java
@Override
public void beforeEach(ExtensionContext context) {
// get the rest catalog client from the context store
Store store = context.getStore(Namespace.GLOBAL);
this.restCatalog = store.get("rest-client");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1823793733
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,18 +94,28 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
- TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
- assertThat(warehouse.delete()).isTrue();
+ public static void setUpAll() {
Review Comment:
```suggestion
public static void setUpAll() throws IOException {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#issuecomment-2468874987
@danielcweeks All comments adopted, feel free to take another look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add REST Catalog tests to Spark 3.5 integration test [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11093:
URL: https://github.com/apache/iceberg/pull/11093#discussion_r1841394148
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java:
##########
@@ -59,20 +87,29 @@ protected static Object[][] parameters() {
}
@BeforeAll
- public static void createWarehouse() throws IOException {
+ public static void setUpAll() throws IOException {
TestBaseWithCatalog.warehouse = File.createTempFile("warehouse", null);
assertThat(warehouse.delete()).isTrue();
+ restCatalog = REST_SERVER_EXTENSION.client();
}
@AfterAll
- public static void dropWarehouse() throws IOException {
+ 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();
}
}
+ static int findFreePort() {
Review Comment:
We should move this into the RESTServerExtension since we can create the client their, we should just configure the client and make it available through the `ExtensionContext`. I don't think we need to leak the details like port into the catalog test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org