feat: Use DH S3Instructions to build Iceberg AWS clients #6113

devinrsmith · 2024-09-23T19:58:31Z

This provides a way for users who are responsible for providing AWS / S3 credentials to specify it in a way where Deephaven can own the S3 client building logic for the Iceberg Catalog in additional to our own data access layer.

Note, this does not deprecate DataInstructionsProviderPlugin, as there may be cases where the user is not responsible for providing these credentials, and it is instead provided via the catalog after catalog authorization. See #6191

extensions/iceberg/s3/src/main/java/io/deephaven/iceberg/util/IcebergToolsS3.java

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3ClientFactory.java

extensions/iceberg/s3/src/main/java/io/deephaven/extensions/s3/DeephavenAwsClientFactory.java

extensions/s3/src/main/java/io/deephaven/extensions/s3/S3ClientFactory.java

extensions/iceberg/s3/src/main/java/io/deephaven/iceberg/util/IcebergToolsS3.java

malhotrashivam · 2024-10-10T19:52:33Z

extensions/iceberg/s3/src/test/java/io/deephaven/iceberg/util/IcebergLocalStackTest.java

@@ -15,9 +16,11 @@
 import static org.apache.iceberg.aws.s3.S3FileIOProperties.ENDPOINT;
 import static org.apache.iceberg.aws.s3.S3FileIOProperties.SECRET_ACCESS_KEY;

+@Tag("testcontainers")
+@Deprecated


I don't understand why we had to deprecate these?
They might still give us coverage on a lot of smaller cases that Larry tested.
Is it deprecated in a sense that new tests should not be added?

It's mostly that I think we should not add new tests here, and work to migrating them as mentioned in IcebergToolsTest. @lbooker42 has so far owned this layer, but it should be relatively easy to migrate to db_resource, and then we get the benefit of:

No separate container (s3) needed (+ no need to upload files)

Ne need to have custom catalog IcebergTestCatalog

On disk JDBC catalog + on disk warehouse

I see db_resource as mainly a way to test out how well we can interoperate with Iceberg that has been written via different processes (pyiceberg, spark, etc).

For more thorough testing (once we have our own writing support) we should be able to extend SqliteCatalogBase (or create further specialized tests that look similar to it), which can work with any warehouse - currently the same logic is tested out via local disk, minio, and localstack.

I agree, these new catalog testing code is the more comprehensive way for all future tests. Once we can migrate these tests so we don't lose any coverage, we should remove these as well as the IcebergTestCatalog class.
cc: @lbooker42

extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergTools.java

malhotrashivam · 2024-10-10T20:15:14Z

extensions/iceberg/s3/src/main/java/io/deephaven/extensions/s3/DeephavenAwsClientFactory.java

    @Override
    public S3Client s3() {
+        checkInit();
        return S3ClientFactory.getSyncClient(instructions);
    }

    @Override
    public GlueClient glue() {


Are the glue, kms and dynamodb clients creation covered under any tests?

No; you could argue we should prefer to throw unsupported for DynamoDB / KMS, but it was easy enough construct those clients if some Iceberg catalog happens to use them (I'm not sure under what circumstances, but it is part of that API...).

I don't know if we've done any Glue testing. https://docs.localstack.cloud/user-guide/aws/glue/ might be possible with the "pro" version, not sure what that entails. https://docs.localstack.cloud/user-guide/aws/glue/

Makes sense, the implementation does seem straightforward and the internal methods are being touched in the S3 code, so I think that part should be okay.

The only thing I am slightly worried about is preloading in case of kms and dynamo clients.
You have a comment stating glue client is preloaded, is it possible to verify the same for kms and dynamo too?

I've done some manual testing of the Glue Catalog with the DeephavenAwsClientFactory, and it works. It was a good exercise though, because I realized that there is a new S3Client built for ever Table load (at least for the Glue catalog), so we can't immediately invoke the cleanup Runnable. After a quick change, it worked.

The only caller I see of kms() is LakeFormationAwsClientFactory - it's possible that some private Catalogs use it, but I'm not sure. It seems like this factory has something do do with Role ARN, and it may not actually be in use anymore since I think Iceberg has expanded how configurable AWS is since the creation of it.

The only caller of dynamodb() is DynamoDbCatalog; it may be possible to test it out with Localstack since it looks to be freely available, but it's just a matter of how much time do we want to spend trying? https://docs.localstack.cloud/user-guide/aws/dynamodb/

Thanks for checking into this.
At this point, maybe just add a TODO comment for kms and dynamodb, for testing it in future?

py/server/deephaven/experimental/iceberg.py

malhotrashivam · 2024-10-10T20:59:21Z

extensions/iceberg/s3/src/main/java/io/deephaven/extensions/s3/DeephavenAwsClientFactory.java

    @Override
    public S3Client s3() {
+        checkInit();
        return S3ClientFactory.getSyncClient(instructions);
    }

    @Override
    public GlueClient glue() {


Makes sense, the implementation does seem straightforward and the internal methods are being touched in the S3 code, so I think that part should be okay.

The only thing I am slightly worried about is preloading in case of kms and dynamo clients.
You have a comment stating glue client is preloaded, is it possible to verify the same for kms and dynamo too?

extensions/iceberg/src/main/java/io/deephaven/iceberg/util/IcebergTools.java

malhotrashivam · 2024-10-10T21:01:36Z

extensions/iceberg/s3/src/test/java/io/deephaven/iceberg/util/IcebergLocalStackTest.java

@@ -15,9 +16,11 @@
 import static org.apache.iceberg.aws.s3.S3FileIOProperties.ENDPOINT;
 import static org.apache.iceberg.aws.s3.S3FileIOProperties.SECRET_ACCESS_KEY;

+@Tag("testcontainers")
+@Deprecated


I agree, these new catalog testing code is the more comprehensive way for all future tests. Once we can migrate these tests so we don't lose any coverage, we should remove these as well as the IcebergTestCatalog class.
cc: @lbooker42

malhotrashivam · 2024-10-11T16:11:27Z

extensions/iceberg/s3/src/main/java/io/deephaven/extensions/s3/DeephavenAwsClientFactory.java

+            throw new IllegalStateException(
+                    "This S3Iinstructions were already cleaned up; please ensure that the returned Runnable from addToProperties is not invoked until the Catalog is no longer in use.");


This is so weird that spotless let this one pass.
Also, typo in S3Instructions.

malhotrashivam · 2024-10-11T16:17:18Z

extensions/iceberg/s3/src/main/java/io/deephaven/iceberg/util/IcebergToolsS3.java

+        CLEANER.register(adapter.catalog(), cleanup);
+        return adapter;


When I had to do something similar for the S3Request objects, Ryan suggested using CleanupReferenceProcessor. You can check that too, if that has any advantages.

feat: Use DH S3Instructions to build Iceberg AWS clients

8066110

devinrsmith added DocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Sep 23, 2024

devinrsmith added this to the 0.37.0 milestone Sep 23, 2024

devinrsmith self-assigned this Sep 23, 2024

Refactor some shared code

4dc52bd

devinrsmith mentioned this pull request Sep 25, 2024

feat: Added support to specify AWS profile through S3 instructions #6130

Merged

devinrsmith marked this pull request as ready for review October 8, 2024 18:07

devinrsmith requested review from lbooker42, malhotrashivam and rcaudy October 8, 2024 18:07

Merge remote-tracking branch 'upstream/main' into dh-iceberg-s3-client

9c13e93

malhotrashivam reviewed Oct 9, 2024

View reviewed changes

Review response

de78d8a

devinrsmith requested a review from malhotrashivam October 10, 2024 00:45

small changes

28f7643

malhotrashivam reviewed Oct 10, 2024

View reviewed changes

Plumb through to iceberg.py

c8ecd2c

devinrsmith requested review from chipkent and jmao-denver as code owners October 10, 2024 20:18

remove import

3804ef9

malhotrashivam reviewed Oct 10, 2024

View reviewed changes

devinrsmith added 2 commits October 10, 2024 14:15

Refactor, don't have IcebergTools extendable

385ac25

Delay cleanup until catalog is unreachable

b909ff3

malhotrashivam reviewed Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use DH S3Instructions to build Iceberg AWS clients #6113

feat: Use DH S3Instructions to build Iceberg AWS clients #6113

devinrsmith commented Sep 23, 2024 •

edited

Loading

malhotrashivam Oct 10, 2024

devinrsmith Oct 10, 2024

malhotrashivam Oct 10, 2024

malhotrashivam Oct 10, 2024

devinrsmith Oct 10, 2024

malhotrashivam Oct 10, 2024

devinrsmith Oct 11, 2024

malhotrashivam Oct 11, 2024 •

edited

Loading

malhotrashivam Oct 10, 2024

malhotrashivam Oct 10, 2024

malhotrashivam Oct 11, 2024 •

edited

Loading

malhotrashivam Oct 11, 2024

		throw new IllegalStateException(
		"This S3Iinstructions were already cleaned up; please ensure that the returned Runnable from addToProperties is not invoked until the Catalog is no longer in use.");

		CLEANER.register(adapter.catalog(), cleanup);
		return adapter;

feat: Use DH S3Instructions to build Iceberg AWS clients #6113

Are you sure you want to change the base?

feat: Use DH S3Instructions to build Iceberg AWS clients #6113

Conversation

devinrsmith commented Sep 23, 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

malhotrashivam 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

malhotrashivam Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith commented Sep 23, 2024 •

edited

Loading

malhotrashivam Oct 11, 2024 •

edited

Loading

malhotrashivam Oct 11, 2024 •

edited

Loading