[#4938]feat(lakehouse-paimon): Support S3 filesystem for Paimon catalog. #4939

yuqi1129 · 2024-09-13T11:30:50Z

What changes were proposed in this pull request?

Add support for Paimon S3 filesystem.

Note: related documents will be added in another PR.

Why are the changes needed?

for better user experience.

Fix: #4938

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Test locally and IT

FANNG1 · 2024-09-14T08:51:54Z

...main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalogPropertiesMetadata.java

@@ -61,6 +62,12 @@ public class PaimonCatalogPropertiesMetadata extends BaseCatalogPropertiesMetada
          AuthenticationConfig.AUTH_TYPE_KEY,
          AuthenticationConfig.AUTH_TYPE_KEY);

+  private static final Map<String, String> S3_CONFIGURATION =
+      ImmutableMap.of(
+          PaimonS3FileSystemConfig.S3_ACCESS_KEY, PaimonS3FileSystemConfig.S3_ACCESS_KEY,


I prefer to use the properties defined in S3Properties to unify the storage configuration for Gravitino

Please see #4939 (comment)

...n/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/FileSystemType.java

...t/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonS3IT.java

FANNG1 · 2024-09-14T08:57:19Z

...t/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonS3IT.java

+  @Override
+  protected void startNecessaryContainers() {
+    localStackContainer =
+        new LocalStackContainer(DockerImageName.parse("localstack/localstack")).withServices(S3);


any reason to use localstack for test.

So, do you have a suggestion for the use of the s3 simulator?

caican00 · 2024-09-15T15:04:04Z

...n/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/FileSystemType.java

+  LOCAL_FILE,
+  HDFS,
+  S3,
+  OSS;


If we do not plan to support oss in this pr, it have better to remove it first to avoid confusion for other users

@FANNG1 Do you have any other suggestions?

seems reasonable to remove OSS, since it's not supported.

...e-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/utils/CatalogUtils.java

caican00 · 2024-09-15T15:20:56Z

...va/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/s3/PaimonS3FileSystemConfig.java

+import org.apache.gravitino.config.ConfigEntry;
+import org.apache.gravitino.connector.PropertyEntry;
+
+public class PaimonS3FileSystemConfig extends Config {


I wonder if it is possible to merge the same configurations of iceberg to provide a common configurations.

The keys for S3 in Iceberg are not the same as those here, please see #4939 (comment)

...t/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonS3IT.java

caican00 · 2024-09-16T02:32:47Z

...n/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/FileSystemType.java

+public enum FileSystemType {
+  LOCAL_FILE,
+  HDFS,
+  S3,


is the rename in S3 an atomic operation?
FilesystemCatalog depends on the atomic rename to avoid commit confliction.

https://github.com/apache/hadoop/blob/ea6e0f7cd58d0129897dfc7870aee188be80a904/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java#L2365-L2385
i am not sure that if it will fail when the destination path exists.

is the rename in S3 an atomic operation? FilesystemCatalog depends on the atomic rename to avoid commit confliction.

I don't have much knowledge about it. let me verify this.

…_4938

yuqi1129 · 2024-09-29T08:05:46Z

@FANNG1
Please help to take a look again.

FANNG1 · 2024-10-08T13:16:11Z

...n/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/FileSystemType.java

+  }
+
+  public static FileSystemType fromStoragePath(String storagePath) {
+    if (storagePath.startsWith("s3://")) {


let me check it for Paimon, the value is s3:// in the example shown in https://paimon.apache.org/docs/0.8/filesystems/s3/

Is the value s3a for Iceberg S3 storage?

FANNG1 · 2024-10-08T13:16:37Z

...n/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/FileSystemType.java

+  }
+
+  public static FileSystemType fromStoragePath(String storagePath) {
+    if (storagePath.startsWith("s3://")) {


shoud we consider upper cases like S3://?

HDFS only support lower case 'hdfs', if we use HDFS://xxx

Caused by: MetaException(message:Got exception: java.io.IOException No FileSystem for scheme: HDFS) at org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$create_database_result$create_database_resultStandardScheme.read(ThriftHiveMetastore.java:26660) at org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$create_database_result$create_database_resultStandardScheme.read(ThriftHiveMetastore.java:26628) at org.apache.hadoop.hive.metastore.api.ThriftHiveMetastore$create_database_result.read(ThriftHiveMetastore.java:26562) at org.apache.thrift.TServiceClient.receiveBase(TServiceClient.java:88)

...e-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/utils/CatalogUtils.java

FANNG1 · 2024-10-08T13:22:31Z

...main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonCatalogPropertiesMetadata.java

+      ImmutableMap.of(
+          S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, S3_ACCESS_KEY,
+          S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY, S3_SECRET_KEY,
+          S3Properties.GRAVITINO_S3_ENDPOINT, S3_ENDPOINT);


should we consider s3 region?

According to the doc provided by Paimon, region is excessive.

...java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonBaseIT.java

FANNG1 · 2024-10-08T13:27:40Z

docs/lakehouse-paimon-catalog.md

+| `authentication.kerberos.keytab-uri`                | The URI of The keytab for the Kerberos authentication.                                                                                                                                                      | (none)                 | required if the value of `authentication.type` is Kerberos.     | 0.6.0         |
+| `authentication.kerberos.check-interval-sec`        | The check interval of Kerberos credential for Paimon catalog.                                                                                                                                               | 60                     | No                                                              | 0.6.0         |
+| `authentication.kerberos.keytab-fetch-timeout-sec`  | The fetch timeout of retrieving Kerberos keytab from `authentication.kerberos.keytab-uri`.                                                                                                                  | 60                     | No                                                              | 0.6.0         |
+| `s3.endpoint`                                       | The endpoint of the AWS s3.                                                                                                                                                                                 | (none)                 | required if the value of `warehouse` is a S3 path               | 0.7.0         |


is the s3 properties name correct?

FANNG1 · 2024-10-09T09:10:30Z

...n/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/FileSystemType.java

+ */
+package org.apache.gravitino.catalog.lakehouse.paimon.filesystem;
+
+public enum FileSystemType {


please remove this class if not used

yuqi1129 added 3 commits September 13, 2024 19:29

Support S3 filesystem for Paimon catalog.

6180ed6

Fix

69e8b28

Fix

1ff547a

yuqi1129 self-assigned this Sep 13, 2024

yuqi1129 added 6 commits September 13, 2024 20:05

Add some document.

f9bf3c1

fix

fe2cc8f

fix test error.

48149dd

Merge branch 'main' of github.com:apache/gravitino into issue_4938

71e7551

FIX

6b040e7

Fix test error in deploy mode.

1836a32

yuqi1129 requested review from caican00 and FANNG1 September 14, 2024 02:48

FANNG1 reviewed Sep 14, 2024

View reviewed changes

...n/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/filesystem/FileSystemType.java Outdated Show resolved Hide resolved

FANNG1 reviewed Sep 14, 2024

View reviewed changes

...t/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonS3IT.java Outdated Show resolved Hide resolved

FANNG1 reviewed Sep 14, 2024

View reviewed changes

caican00 reviewed Sep 15, 2024

View reviewed changes

...e-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/utils/CatalogUtils.java Outdated Show resolved Hide resolved

caican00 reviewed Sep 15, 2024

View reviewed changes

...t/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonS3IT.java Outdated Show resolved Hide resolved

caican00 reviewed Sep 16, 2024

View reviewed changes

yuqi1129 added 8 commits September 18, 2024 19:53

Fix

faf5e59

Merge branch 'main' of github.com:apache/gravitino into issue_4938

acd6b1a

Resolve comments

56358cb

forget to push file S3StorageConfig

4229cdb

Merge branch 'issue_4938' of github.com:yuqi1129/gravitino into issue…

04bf3d6

…_4938

Merge branch 'main' of github.com:datastrato/graviton into issue_4938

dfcb374

Merge branch 'issue_4938' of github.com:yuqi1129/gravitino into issue…

6daf940

…_4938

Fix

3c21686

yuqi1129 requested a review from FANNG1 September 24, 2024 13:46

yuqi1129 added 4 commits September 24, 2024 23:01

Fix

49f9ac3

Fix

6135e7e

Merge branch 'main' of github.com:datastrato/graviton into issue_4938

f5a2a30

fix

15e6ca9

FANNG1 reviewed Oct 8, 2024

View reviewed changes

...e-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/utils/CatalogUtils.java Outdated Show resolved Hide resolved

FANNG1 reviewed Oct 8, 2024

View reviewed changes

...java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonBaseIT.java Outdated Show resolved Hide resolved

FANNG1 reviewed Oct 8, 2024

View reviewed changes

yuqi1129 added 2 commits October 9, 2024 11:59

Merge branch 'main' of github.com:datastrato/graviton into issue_4938

d68bd65

Fix

df823d9

yuqi1129 requested a review from FANNG1 October 9, 2024 06:37

yuqi1129 closed this Oct 9, 2024

yuqi1129 reopened this Oct 9, 2024

FANNG1 reviewed Oct 9, 2024

View reviewed changes

Removed unused class.

c705667

FANNG1 approved these changes Oct 10, 2024

View reviewed changes

FANNG1 merged commit f753afa into apache:main Oct 10, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4938]feat(lakehouse-paimon): Support S3 filesystem for Paimon catalog. #4939

[#4938]feat(lakehouse-paimon): Support S3 filesystem for Paimon catalog. #4939

yuqi1129 commented Sep 13, 2024

FANNG1 Sep 14, 2024

yuqi1129 Sep 18, 2024

FANNG1 Sep 14, 2024

yuqi1129 Sep 14, 2024

caican00 Sep 15, 2024

yuqi1129 Sep 18, 2024

FANNG1 Sep 18, 2024

caican00 Sep 15, 2024

yuqi1129 Sep 18, 2024

caican00 Sep 16, 2024

caican00 Sep 16, 2024

yuqi1129 Sep 18, 2024

yuqi1129 commented Sep 29, 2024

FANNG1 Oct 8, 2024

yuqi1129 Oct 9, 2024 •

edited

Loading

yuqi1129 Oct 9, 2024

FANNG1 Oct 8, 2024

yuqi1129 Oct 9, 2024

FANNG1 Oct 8, 2024

yuqi1129 Oct 9, 2024

FANNG1 Oct 8, 2024

yuqi1129 Oct 9, 2024

FANNG1 Oct 9, 2024

[#4938]feat(lakehouse-paimon): Support S3 filesystem for Paimon catalog. #4939

[#4938]feat(lakehouse-paimon): Support S3 filesystem for Paimon catalog. #4939

Conversation

yuqi1129 commented Sep 13, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

yuqi1129 commented Sep 29, 2024

Choose a reason for hiding this comment

yuqi1129 Oct 9, 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

yuqi1129 Oct 9, 2024 •

edited

Loading