Add REST Catalog tests to Spark 3.5 integration test #11093

haizhou-zhao · 2024-09-06T21:17:59Z

For issue: #11079

Add REST Catalog tests to Spark 3.4 integration test tmp save Fix integ tests Revert "Add REST Catalog tests to Spark 3.4 integration test" This reverts commit d052416. unneeded changes fix test retrigger checks Fix integ test Fix port already in use Fix unmatched validation catalog spotless Fix sqlite related test failures

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java

nastra · 2024-10-08T08:49:05Z

...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();


is this refresh only needed for REST?

Yes, RESTTableOperations and other TableOperations has different mechanisms of refreshing metadata.

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

nastra · 2024-10-08T08:58:42Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

+      // 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");


is this strictly needed to make tests pass? I don't think we set this in any other tests for that specific purpose

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

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

nastra · 2024-10-11T07:23:59Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

+      System.setProperty(CatalogProperties.CLIENT_POOL_SIZE, "1");
+      restServer.start(false);
+      restCatalog = RCKUtils.initCatalogClient();
+      System.clearProperty("rest.port");


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?

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

nastra · 2024-10-11T07:32:07Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

  @BeforeEach
  public void before() {
-    this.validationCatalog =


what happens if we don't do any changes to how the validation catalog is configured?

something like you created a table on REST catalog, while validate against Hive catalog.

my point is that I think all tests should be passing when we don't do any changes to the validation catalog

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.

nastra · 2024-10-11T07:33:29Z

open-api/src/testFixtures/java/org/apache/iceberg/rest/RESTCatalogServer.java

-    Map<String, String> catalogProperties = RCKUtils.environmentCatalogConfig();
+    Map<String, String> catalogProperties = Maps.newHashMap();
+    catalogProperties.putAll(RCKUtils.environmentCatalogConfig());
+    catalogProperties.putAll(Maps.fromProperties(System.getProperties()));


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)

that means we can revert this line here: catalogProperties.putAll(Maps.fromProperties(System.getProperties()));

This reverts commit ae29c41.

haizhou-zhao · 2024-10-11T21:31:03Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

+    // 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");


There was this conversation getting lost (not sure why): #11093 (comment)

…seWithCatalog.java Co-authored-by: Eduard Tudenhoefner <[email protected]>

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

nastra · 2024-10-31T05:41:43Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

+      new RESTServerExtension(
+          Map.of(
+              RESTCatalogServer.REST_PORT, String.valueOf(RCKUtils.findFreePort()),
+              CatalogProperties.WAREHOUSE_LOCATION, warehouse.getAbsolutePath(),


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

…seWithCatalog.java

danielcweeks · 2024-10-31T15:46:19Z

open-api/src/testFixtures/java/org/apache/iceberg/rest/RCKUtils.java

+      port = socket.getLocalPort();
+      socket.close();
+    } catch (IOException e) {
+      throw new RuntimeException(e);


Nit: This could use try-with-resources and we should probably use UncheckedIOException rather than RE.

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.

danielcweeks · 2024-10-31T16:03:21Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java

+  }
+
+  private static void initRESTCatalog() {
+    restCatalog = RCKUtils.initCatalogClient(REST_SERVER_EXTENSION.config());


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:

// 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

@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"); }

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.

danielcweeks · 2024-10-31T16:05:59Z

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() {


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.

danielcweeks · 2024-10-31T16:07:44Z

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.

github-actions bot added spark build OPENAPI labels Sep 6, 2024

haizhou-zhao force-pushed the spark-rest-integ-test branch from f01f8e5 to 26f3d99 Compare September 16, 2024 18:59

Haizhou Zhao added 2 commits September 18, 2024 15:44

Rebase & spotless

8bcd853

haizhou-zhao force-pushed the spark-rest-integ-test branch from 9481bd4 to 8bcd853 Compare September 18, 2024 23:18

nastra reviewed Oct 8, 2024

View reviewed changes

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java Outdated Show resolved Hide resolved

nastra reviewed Oct 8, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 8, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 8, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Show resolved Hide resolved

nastra reviewed Oct 8, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java Outdated Show resolved Hide resolved

nastra reviewed Oct 8, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 8, 2024

View reviewed changes

Haizhou Zhao added 2 commits October 10, 2024 12:54

code format

fc8abfb

unneeded change

60aafd1

nastra reviewed Oct 11, 2024

View reviewed changes

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java Outdated Show resolved Hide resolved

nastra reviewed Oct 11, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestAlterTable.java Outdated Show resolved Hide resolved

nastra reviewed Oct 11, 2024

View reviewed changes

...5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMetadataTables.java Outdated Show resolved Hide resolved

nastra reviewed Oct 11, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 11, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 11, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 11, 2024

View reviewed changes

Haizhou Zhao added 3 commits October 11, 2024 10:57

unneeded change

ae29c41

Revert "unneeded change"

4c26473

This reverts commit ae29c41.

code format

140382e

haizhou-zhao commented Oct 11, 2024

View reviewed changes

haizhou-zhao and others added 2 commits October 30, 2024 10:54

Update spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBa…

d872fab

…seWithCatalog.java Co-authored-by: Eduard Tudenhoefner <[email protected]>

Spotless and fix test

2a96d03

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

Apply suggestions from code review

277f1d1

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

Apply suggestions from code review

e44e55f

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

Apply suggestions from code review

4ed12a6

nastra requested a review from danielcweeks October 31, 2024 05:48

nastra reviewed Oct 31, 2024

View reviewed changes

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBaseWithCatalog.java Outdated Show resolved Hide resolved

Update spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/TestBa…

9cd1dc6

…seWithCatalog.java

nastra approved these changes Oct 31, 2024

View reviewed changes

danielcweeks reviewed Oct 31, 2024

View reviewed changes

Haizhou Zhao added 5 commits October 31, 2024 11:48

Package protected RCKUtils

706c511

spotless

77c661f

unintentional change

c4661a5

remove warehouse specification from rest

6ee7df8

spotless

ba1e187

nastra mentioned this pull request Nov 5, 2024

Spark: add property to disable client-side purging in spark #11317

Open

nastra requested a review from danielcweeks November 5, 2024 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add REST Catalog tests to Spark 3.5 integration test #11093

Add REST Catalog tests to Spark 3.5 integration test #11093

haizhou-zhao commented Sep 6, 2024

nastra Oct 8, 2024

haizhou-zhao Oct 10, 2024

nastra Oct 8, 2024

haizhou-zhao Oct 11, 2024

nastra Oct 11, 2024 •

edited

Loading

nastra Oct 11, 2024

haizhou-zhao Oct 11, 2024

nastra Oct 14, 2024

haizhou-zhao Oct 14, 2024

nastra Oct 11, 2024

haizhou-zhao Oct 14, 2024

nastra Oct 23, 2024

haizhou-zhao Oct 29, 2024

haizhou-zhao Oct 11, 2024

nastra Oct 31, 2024

haizhou-zhao Oct 31, 2024

danielcweeks Oct 31, 2024

haizhou-zhao Oct 31, 2024

haizhou-zhao Oct 31, 2024

danielcweeks Oct 31, 2024 •

edited

Loading

haizhou-zhao Oct 31, 2024

danielcweeks Oct 31, 2024

haizhou-zhao Oct 31, 2024

danielcweeks commented Oct 31, 2024

Add REST Catalog tests to Spark 3.5 integration test #11093

Are you sure you want to change the base?

Add REST Catalog tests to Spark 3.5 integration test #11093

Conversation

haizhou-zhao commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcweeks Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcweeks commented Oct 31, 2024

nastra Oct 11, 2024 •

edited

Loading

danielcweeks Oct 31, 2024 •

edited

Loading