add lakekeeper integration test #25140

twuebi · 2025-02-25T15:39:52Z

Description

This PR adds tests for the Lakekeeper Iceberg catalog.

Additional context and related issues

Lakekeeper does not support storing metadata on local file storage, this means we need to run against a full deployment, which includes minio, postgres and the catalog itself. While a bit more maintenance, it also means that these tests are close to what users will run. A few tests had to be disabled, each of the disabled tests should have a short explanation.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

cla-bot · 2025-02-25T15:39:57Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-02-25T16:38:42Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

ebyhr

add lakekeeper integration test

The commit message should start with uppercase.
https://trino.io/development/process#pull-request-and-commit-guidelines-

ebyhr · 2025-02-25T22:13:54Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java

@@ -585,6 +585,7 @@ public void testDropTableWithMissingMetadataFile()
        // try to drop table
        assertUpdate("DROP TABLE " + tableName);
        assertThat(getQueryRunner().tableExists(getSession(), tableName)).isFalse();
+


Please revert unrelated changes.

ebyhr · 2025-02-25T22:14:36Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergQueryRunner.java

@@ -226,6 +227,42 @@ public static void main(String[] args)
        }
    }

+    public static final class IcebergLakekeeperQueryRunnerMain {


Please follow our codestyle: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#code-style

ebyhr · 2025-02-26T02:21:08Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+
+    }
+
+    public void drop_without_purge(String schema, String table) {


Rename to dropTableWithoutPurge.

ebyhr · 2025-02-26T02:27:10Z

...ava/io/trino/plugin/iceberg/catalog/rest/TestIcebergLakekeeperCatalogConnectorSmokeTest.java

+    @Override
+    protected QueryRunner createQueryRunner()
+            throws Exception {
+        lakekeeperCatalog = closeAfterClass(new TestingLakekeeperCatalog("125ms"));


Why do we use a different poll interval between TestIcebergLakekeeperCatalogConnectorSmokeTest and IcebergLakekeeperQueryRunnerMain?

There are a few tests that check that files are gone after dropping. Since Lakekeeper cleans up files async and returns a response before the clean-up is done, we need to sleep in the tests. The polling interval determines how long we have to sleep. A smaller poll interval means tests run faster but it also makes the Lakekeeper instance chattier. In IcebergLakekeeperQueryRunnerMain, we dump the logs if an exception occurs, if we have a small poll interval there, the logs become quite unreadable, at the same time, we don't need the almost-instant cleanup in IcebergLakekeeperQueryRunnerMain since there are no tests checking if files are actually gone.

ebyhr · 2025-02-26T02:31:12Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+        String lakekeeperDbUrlReadKey = "LAKEKEEPER__PG_DATABASE_URL_READ";
+        String lakekeeperDbUrlWriteKey = "LAKEKEEPER__PG_DATABASE_URL_WRITE";
+        String lakekeeperPGEncryptionKey = "LAKEKEEPER__PG_ENCRYPTION_KEY";
+        String lakekeeperCatalogImage = "quay.io/lakekeeper/catalog:v0.7.0";
+        String pgEncryptionKey = "This-is-NOT-Secure!";


Move these variables to fields as constants.

ebyhr · 2025-02-26T02:31:47Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+    private final GenericContainer<?> migrator;
+    private final PostgreSQLContainer<?> postgresqlContainer;
+    private final GenericContainer<?> minio;
+    private final Network net;


Rename to network.

ebyhr · 2025-02-26T02:32:47Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+    public static final String MINIO_ROOT_USER = "minio-root-user";
+    public static final String MINIO_ROOT_PASSWORD = "minio-root-password";


Use same values as io.trino.testing.containers.Minio:

Suggested change

public static final String MINIO_ROOT_USER = "minio-root-user";

public static final String MINIO_ROOT_PASSWORD = "minio-root-password";

public static final String MINIO_ROOT_USER = "accesskey";

public static final String MINIO_ROOT_PASSWORD = "secretkey";

ebyhr · 2025-02-26T02:34:28Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+    public String getLogs() {
+        return lakekeeperCatalog.getLogs();
+    }


Inline this method.

This method is currently used from IcebergLakekeeperQueryRunnerMain. In case an exception occurs, we print the logs over there. We can remove the log-dump, then we can also remove this method altogether.

ebyhr · 2025-02-26T02:36:04Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+    public int minioPort() {
+        return this.minio.getMappedPort(MINIO_PORT);
+    }


We could change to minioEndpoint that returns "http://localhost:" + minio.getMappedPort(MINIO_PORT).

ebyhr · 2025-02-26T02:36:25Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+                .withEnv(lakekeeperDbUrlWriteKey, pgConnectionString).withNetwork(postgresqlContainer.getNetwork())
+                .withCommand("migrate").start();
+
+        minio = new GenericContainer<>("bitnami/minio:latest");


Avoid using latest tag. It leads to flaky tests.

ebyhr · 2025-02-26T02:37:02Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestingLakekeeperCatalog.java

+        postgresqlContainer = new PostgreSQLContainer<>("postgres:16");
+        postgresqlContainer.withNetwork(net).start();
+
+        String pgConnectionString = "postgresql://" + postgresqlContainer.getUsername() + ":" + postgresqlContainer.getPassword() + "@" + postgresqlContainer.getContainerName().substring(1) + ":" + PostgreSQLContainer.POSTGRESQL_PORT + "/" + postgresqlContainer.getDatabaseName();


This line is too long.

ebyhr · 2025-02-26T03:14:14Z

@twuebi Could you explain why we should test Lakekeeper in this repository? We already test several REST catalog implementations (Unity, Polaris, S3 Tables and Tabular). I feel they are sufficient to ensure Iceberg REST catalog specification.

cla-bot · 2025-02-26T09:10:15Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-02-26T10:00:18Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-02-26T11:01:08Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-02-26T11:03:33Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

mosabua · 2025-02-26T16:49:57Z

@twuebi Could you explain why we should test Lakekeeper in this repository? We already test several REST catalog implementations (Unity, Polaris, S3 Tables and Tabular). I feel they are sufficient to ensure Iceberg REST catalog specification.

I think the situation is similar to S3 but with the difference that the Iceberg REST Catalog spec is more formal. The fact remains that we want to ensure that as many different REST catalogs can show that they work with Trino and do so with every release. The current way to ensure that is to have them run tests here.. just like we do with minio and S3 and so on.

We can discuss this more in the maintainer call since it would be good to have a better guideline.

This work is also related to the fact that we want to mention official support from Trino for these projects and there is a web site PR to show this better. We also want to expand the docs for the catalogs and I am hoping @ebyhr @findinpath and others can help over time.

The first steps are this PR and the website PR .. that still needs your technical review and input btw. trinodb/trino.io#770

More to come

ebyhr · 2025-02-27T05:58:31Z

I think the situation is similar to S3 but with the difference that the Iceberg REST Catalog spec is more formal.

I think so too. It's a spec Lakekeeper should follow.
Note that even just adding tests increases our (≠their) maintenance costs.

We can discuss this more in the maintainer call since it would be good to have a better guideline.

I recommend talking in Slack so other people can chime in. I personally don't want to support projects (#stars < 1k) unless there is a justification.

cla-bot · 2025-02-28T10:47:30Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

github-actions bot added the iceberg Iceberg connector label Feb 25, 2025

twuebi force-pushed the tp/lakekeeper-tests branch from 11de3df to 5878f9b Compare February 25, 2025 16:38

ebyhr reviewed Feb 25, 2025

View reviewed changes

ebyhr reviewed Feb 26, 2025

View reviewed changes

twuebi force-pushed the tp/lakekeeper-tests branch from b3c4f5d to 1ed80de Compare February 26, 2025 10:00

c-thiel mentioned this pull request Feb 26, 2025

fix: Client Credential Authentication for OpenFGA, allow to configure Scopes lakekeeper/lakekeeper#863

Merged

twuebi force-pushed the tp/lakekeeper-tests branch from 638ed47 to 11e8e26 Compare February 26, 2025 11:03

add lakekeeper integration test

7d3fdf3

twuebi force-pushed the tp/lakekeeper-tests branch from 11e8e26 to 7d3fdf3 Compare February 28, 2025 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add lakekeeper integration test #25140

add lakekeeper integration test #25140

twuebi commented Feb 25, 2025 •

edited by ebyhr

Loading

cla-bot bot commented Feb 25, 2025

cla-bot bot commented Feb 25, 2025

ebyhr left a comment

ebyhr Feb 25, 2025

ebyhr Feb 25, 2025

ebyhr Feb 26, 2025

ebyhr Feb 26, 2025

twuebi Feb 26, 2025

ebyhr Feb 26, 2025

ebyhr Feb 26, 2025

ebyhr Feb 26, 2025

ebyhr Feb 26, 2025

twuebi Feb 26, 2025

ebyhr Feb 26, 2025

ebyhr Feb 26, 2025

ebyhr Feb 26, 2025

ebyhr commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

mosabua commented Feb 26, 2025

ebyhr commented Feb 27, 2025 •

edited

Loading

cla-bot bot commented Feb 28, 2025


		}

		public void drop_without_purge(String schema, String table) {

		public static final String MINIO_ROOT_USER = "minio-root-user";
		public static final String MINIO_ROOT_PASSWORD = "minio-root-password";

add lakekeeper integration test #25140

Are you sure you want to change the base?

add lakekeeper integration test #25140

Conversation

twuebi commented Feb 25, 2025 • edited by ebyhr Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Feb 25, 2025

cla-bot bot commented Feb 25, 2025

ebyhr left a 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

ebyhr commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

cla-bot bot commented Feb 26, 2025

mosabua commented Feb 26, 2025

ebyhr commented Feb 27, 2025 • edited Loading

cla-bot bot commented Feb 28, 2025

twuebi commented Feb 25, 2025 •

edited by ebyhr

Loading

ebyhr commented Feb 27, 2025 •

edited

Loading