[#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)

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

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

+    throw new IllegalArgumentException("Unsupported file system type: " + type);
+  }
+
+  public static FileSystemType fromStoragePath(String storagePath) {


seems couldn't be extended to support other storage implement FILEIO

let me check it.

Noticed by @FANNG1, The Iceberg catalog uses gravitino.bypass mechanism to transfer properties to the underlying storage, thus making it very good scalability and can support other storage system without changing the code, so I wonder

Should we follow this design for the Paimon catalog to support multiple file systems storage?

Is it advantageous to adopt this strategy for the fileset catalog to support multiple storage?

@FANNG1 @jerryshao

I would suggest to leverage the configurations defined by @FANNG1 for Iceberg in Paimon too, we can unify the cloud storage configuration for different catalogs.

For other configurations that Gravitino cannot support, we can use "gravitino.bypass". But for some important configurations we should have a unified manner in Gravitino.

I received it and checked it before launching the PR. The keys they used were not necessarily the same.

public class S3Properties { // An alternative endpoint of the S3 service, This could be used to for S3FileIO with any // s3-compatible object storage service that has a different endpoint, or access a private S3 // endpoint in a virtual private cloud public static final String GRAVITINO_S3_ENDPOINT = "s3-endpoint"; // The static access key ID used to access S3 data. public static final String GRAVITINO_S3_ACCESS_KEY_ID = "s3-access-key-id"; // The static secret access key used to access S3 data. public static final String GRAVITINO_S3_SECRET_ACCESS_KEY = "s3-secret-access-key"; // The region of the S3 service. public static final String GRAVITINO_S3_REGION = "s3-region"; private S3Properties() {} }

public class PaimonS3FileSystemConfig extends Config { public static final String S3_ENDPOINT = "s3.endpoint"; public static final String S3_ACCESS_KEY = "s3.access-key"; public static final String S3_SECRET_KEY = "s3.secret-key"; }

Different catalog may use different S3 properties, like s3.access-key-id in Iceberg, s3.access-key in Paimon, and XX in hadoop, etc. Given this variety, it seems reasonable to propose a unified set of cloud properties. This approach would simplify the usage of S3 across various catalogs for normal users.

Yeah, it should be the same.

We should unify all the cloud storage related configurations and provide a unified way to the users, hide the difference for different catalogs.

This is important, should carefully think about it and treat it seriously.

FANNG1 · 2024-09-14T08:56:13Z

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

+  }
+
+  @AfterAll
+  public void stop() {


stop spark session

It should be stop in the super stop method, let me check

Yeah, the spark session has been closed in the super class.

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.

caican00 · 2024-09-15T15:17:34Z

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

+    }
+  }
+
+  private static void checkS3FileSystemConfig(Map<String, String> resultConf) {


this check looks repeated.
https://github.com/apache/gravitino/pull/4939/files#diff-0c775b587b52f59019d196d56c6de96c190a6320085c91ad7757e5ac8255bef5R54

https://github.com/apache/gravitino/pull/4939/files#diff-0c775b587b52f59019d196d56c6de96c190a6320085c91ad7757e5ac8255bef5R54 aims to solve the problem that if the users set the key, then the values should not be null or empty. If the key is not set, the check is useless.

This is to address the problem that users should set the key and the value can't be null or empty.

got it, I misunderstood.

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)

caican00 · 2024-09-15T15:34:42Z

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

+        String serverPath = ITUtils.joinPath(gravitinoHome, "libs");
+        String paimonCatalogPath =
+            ITUtils.joinPath(gravitinoHome, "catalogs", "lakehouse-paimon", "libs");
+        JdbcDriverDownloader.downloadJdbcDriver(PAIMON_S3_JAR_URL, serverPath, paimonCatalogPath);


JdbcDriverDownloader.downloadJdbcDriver

Should we adjust the method name to make it seem compatible with downloading any jar?

Yeah, you are right, I need to rename it.

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.

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

caican00 reviewed Sep 15, 2024

View reviewed changes

caican00 reviewed Sep 16, 2024

View reviewed changes

Fix

faf5e59

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

yuqi1129 Sep 18, 2024

jerryshao Sep 18, 2024

yuqi1129 Sep 18, 2024

FANNG1 Sep 18, 2024 •

edited

Loading

jerryshao Sep 19, 2024 •

edited

Loading

FANNG1 Sep 14, 2024

yuqi1129 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 18, 2024

caican00 Sep 15, 2024

yuqi1129 Sep 18, 2024

caican00 Sep 15, 2024

yuqi1129 Sep 18, 2024

caican00 Sep 16, 2024

caican00 Sep 16, 2024

yuqi1129 Sep 18, 2024

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

Are you sure you want to change the base?

[#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

FANNG1 Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

jerryshao Sep 19, 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

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

FANNG1 Sep 18, 2024 •

edited

Loading

jerryshao Sep 19, 2024 •

edited

Loading