[#5220] improvment(hadoop-catalog): Optimize the name properties keys for Hadoop catalog. #5244

yuqi1129 · 2024-10-24T14:43:12Z

What changes were proposed in this pull request?

Replace the properties keys with gravitino.bypass prefix with a more elegant one.

Why are the changes needed?

For better user experience.

Fix: #5220

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Existing UTs and ITs.

…fileset catalog

catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/S3Properties.java

...adoop/src/main/java/org/apache/gravitino/catalog/hadoop/config/HadoopS3FileSystemConfig.java

catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/GCSProperties.java

catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/S3Properties.java

...atalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java

...doop/src/main/java/org/apache/gravitino/catalog/hadoop/config/HadoopGCSFileSystemConfig.java

jerryshao · 2024-10-28T14:13:16Z

...p/src/test/java/org/apache/gravitino/catalog/hadoop/integration/test/HadoopGCSCatalogIT.java

@@ -95,10 +96,8 @@ protected String defaultBaseLocation() {

  protected void createCatalog() {


Can we move these ITs to each bundle?

Yes, I can try it but I will not do it in this PR, It will make this PR large and hard to test the changes introduced by this PR, so I created #5325 to follow it.

jerryshao

I think you should think if user implement a custom FS with FS provider, can he integrate into Gravitino server Hadoop catalog, Java and Python gvfs without code change in Gravitino? Take this as a principle, if not, please make it work.

bundles/aliyun-bundle/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java

jerryshao · 2024-10-29T06:13:30Z

bundles/aliyun-bundle/src/main/java/org/apache/gravitino/oss/fs/OSSFileSystemProvider.java

+
+  private static final String OSS_FILESYSTEM_IMPL = "fs.oss.impl";
+
+  public static final Map<String, String> GRAVITINO_KEY_TO_OSS_HADOOP_KEY =


Why do we want to make this public?

Again, why do we turn it back to public?

This value will be used in GravitinoVirtualFileSystemOSSIT test and others are similar.

This is because you don't put IT and code together, so you have to make it public, can you please add a visible for test tag on it?

bundles/gcp-bundle/src/main/java/org/apache/gravitino/gcs/fs/GCSFileSystemProvider.java

bundles/gcp-bundle/build.gradle.kts

jerryshao · 2024-10-29T06:18:46Z

.../catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/FileSystemProvider.java

@@ -32,6 +32,8 @@
 */
 public interface FileSystemProvider {

+  String GRAVITINO_BYPASS = "gravitino.bypass.";


Can you please add some comments to this variable, to let users know how to leverage this variable.

jerryshao · 2024-10-29T06:23:10Z

...ain/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystemConfiguration.java

@@ -107,5 +112,46 @@ public class GravitinoVirtualFileSystemConfiguration {
  public static final long FS_GRAVITINO_FILESET_CACHE_EVICTION_MILLS_AFTER_ACCESS_DEFAULT =
      1000L * 60 * 60;

+  private static final Map<String, String> GVFS_S3_KEY_TO_HADOOP_KEY =


If we add a new FS provider, do we need to update the GVFS code to make it work? If so, I think it is not a good design, we should also hide the client side configuration into each provider.

In the long run, we can provide a method like requiredPropertyKey in the FileSystemProvider and load this provider by service provider. By doing so, we can register this property to the client dynamically. Currently, we cannot avoid this hard coding.

I still suggest to move this to each provider, we don't have to expose them here.

The fact that this will make module hadoop3-filesystem depend on bundle jars in the runtime explicitly will hinder me from doing so. If this is not a big problem, I would like to do so.

Why there is a dependency? If user sets the configurations via Hadoop Configuration in gvfs, when gvfs create a provider, it can pass the configs from gvfs' Configuration to provider's config map. Each provider can use this config map to check the configurations and initialize the FS.

yuqi1129 · 2024-10-29T07:25:15Z

I think you should think if user implement a custom FS with FS provider, can he integrate into Gravitino server Hadoop catalog, Java and Python gvfs without code change in Gravitino? Take this as a principle, if not, please make it work.

In most cases, the current implementation can satisfy users' needs. They can use the gravitino.bypass mechanism to pass to keys with prefixes gravitino.bypass to the filesystem providers, the file system provider then will remove the prefix and get the real keys. or just directly pass the properties.

For example, if we use the following key/value pair as the properties when creating a Hadoop catalog,

k1=value1
gravitino.bypass.k2=value2
....

Then, we will pass these configurations on to the file system provider. It's quite flexible now and it depends on the users that custom a new file system provider what a property key stands for. key k1 may stand for a core configuration or just a useless one.

k1=value1
k2=value2

The following three keys are identical and users can use any key that contains endpoint information to the file system provider and then assign it to the key fs.s3a.endpoint

fs.s3a.endpoint = xxxxx
s3-endpoint=xxxx
gravitino.bypass.fs.s3a.endpoint = xxxx

I'm not very sure whether the kinds of property conversion are acceptable to you.

If we need to specify each required property explicitly, like the current S3 implementation, users need to use s3-endpoint in the Gravitino server and s3.endpoint in the GVFS Java client to pass endpoint information, I believe we need to add a new interface method like requireConfigurationKey() to tell users what the properties they required to pass, I'm hesitant to do so as it seems to make thing more complex and not very extensive enough.

fix

jerryshao · 2024-10-29T10:34:52Z

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/FileSystemUtils.java

+    config.forEach(
+        (k, v) -> {
+          if (predefinedKeys.containsKey(k)) {
+            String key = predefinedKeys.get(k);
+            highestPriorityKeys.add(key);
+            result.put(key, v);
+          }
+
+          if (!k.startsWith(GRAVITINO_BYPASS)) {
+            // If the key does not start with 'gravitino.bypass' and is not in the highest priority
+            // keys, set the value to the configuration.
+            if (!highestPriorityKeys.contains(k)) {
+              result.put(k, v);
+            }
+          } else {
+            // If the key starts with 'gravitino.bypass', remove the prefix and set the value to the
+            // configuration if the key does not exist in the configuration.
+            String key = k.replace(GRAVITINO_BYPASS, "");
+            if (!result.containsKey(key)) {
+              result.put(key, v);
+            }
+          }
+        });


Can we simplify this code? It's a bit hard to understand, I think it is OK to iterate for several rounds, just make it easy to read.

Can you add a UT for this?

yuqi1129 · 2024-10-29T09:55:28Z

.../catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/FileSystemProvider.java

@@ -32,6 +32,8 @@
 */
 public interface FileSystemProvider {

+  String GRAVITINO_BYPASS = "gravitino.bypass.";


yuqi1129 · 2024-10-29T09:56:55Z

...p/src/test/java/org/apache/gravitino/catalog/hadoop/integration/test/HadoopGCSCatalogIT.java

@@ -107,4 +112,64 @@ protected void createCatalog() {
  protected String generateLocation(String filesetName) {
    return String.format("%s/%s", defaultBaseLocation, filesetName);
  }
+
+  @Test
+  public void testCreateSchemaAndFilesetWithSpecialLocation() {


This is to fix the bug introduced by #5296, we need to consider other Hadoop catalog like S3, OSS and GCS.

yuqi1129 · 2024-10-29T10:00:17Z

...ain/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystemConfiguration.java

@@ -107,5 +112,46 @@ public class GravitinoVirtualFileSystemConfiguration {
  public static final long FS_GRAVITINO_FILESET_CACHE_EVICTION_MILLS_AFTER_ACCESS_DEFAULT =
      1000L * 60 * 60;

+  private static final Map<String, String> GVFS_S3_KEY_TO_HADOOP_KEY =


In the long run, we can provide a method like requiredPropertyKey in the FileSystemProvider and load this provider by service provider. By doing so, we can register this property to the client dynamically. Currently, we cannot avoid this hard coding.

jerryshao · 2024-10-29T13:51:39Z

...hadoop3/src/main/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystem.java

                FileSystemProvider provider = fileSystemProvidersMap.get(scheme);
                if (provider == null) {
                  throw new GravitinoRuntimeException(
                      "Unsupported file system scheme: %s for %s.",
                      scheme, GravitinoVirtualFileSystemConfiguration.GVFS_SCHEME);
                }
+
+                Map<String, String> maps = getConfigMap(getConf());


What is the gvfs configuration for now, is it aligned with server side conf, or still use "."? I cannot find the code anymore.

They are the same as those in Gravitino server side.

Have you verified manually using gvfs?

Yes , GravitinoVirtualFileSystemS3IT can be running in the CI and cover the changes.

…rovider and add more comments.

jerryshao · 2024-10-29T14:29:51Z

Overall LGTM, please fix the ci issue.

… for Hadoop catalog. (#5244) ### What changes were proposed in this pull request? Replace the properties keys with `gravitino.bypass` prefix with a more elegant one. ### Why are the changes needed? For better user experience. Fix: #5220 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Existing UTs and ITs.

…s keys for Hadoop catalog. (apache#5244) ### What changes were proposed in this pull request? Replace the properties keys with `gravitino.bypass` prefix with a more elegant one. ### Why are the changes needed? For better user experience. Fix: apache#5220 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Existing UTs and ITs.

yuqi1129 and others added 30 commits August 26, 2024 23:11

Fix minor in docker image.

ce3b19f

Merge branch 'apache:main' into main

cd69c33

Add a framework to support multi-storage in a pluginized manner for …

d2447a2

…fileset catalog

Fix compile distribution error.

7e5a8b5

fix

f53c5ef

fix

36fedcd

fix

e93fba5

fix

b1e04b6

Changed according to comments.

db00e65

fix

c793582

fix

013f5cb

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

dba5753

resolve comments.

16dfc73

Polish code.

278fcd8

fix

3fb55ad

fix

cd04666

Support GCS fileset.

d0bf13e

Change gvfs accordingly.

ffaa064

Merge remote-tracking branch 'me/issue_5019' into issue_5019

32d7f3d

Merge branch 'apache:main' into main

81a36ba

Update Java doc for FileSystemProvider

d82bf76

Fix

dfdb772

Fix

8708a8a

Fix test error.

ba9f8fa

Polish.

dae99f7

Polish

e22053b

Polish

4fb89e0

Rename AbstractIT to BaseIT

e5746c0

Fix

b2d7bed

Merge branch 'apache:main' into issue_5019

380717b

yuqi1129 added 2 commits October 25, 2024 09:49

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

ca57d3f

fix

10b6267

jerryshao reviewed Oct 25, 2024

View reviewed changes

catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/S3Properties.java Show resolved Hide resolved

jerryshao reviewed Oct 25, 2024

View reviewed changes

...adoop/src/main/java/org/apache/gravitino/catalog/hadoop/config/HadoopS3FileSystemConfig.java Outdated Show resolved Hide resolved

jerryshao reviewed Oct 25, 2024

View reviewed changes

change python keys to make it align with java

03bc296

jerryshao reviewed Oct 28, 2024

View reviewed changes

yuqi1129 added 3 commits October 29, 2024 11:22

Update.

32d6553

Merge branch 'main' of github.com:yuqi1129/gravitino into issue_5220

11c9f41

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

6192586

jerryshao reviewed Oct 29, 2024

View reviewed changes

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

67ac175

yuqi1129 added 2 commits October 29, 2024 17:54

fix

268831d

fix

Optimize.

27393e5

yuqi1129 dismissed a stale review via 27393e5 October 29, 2024 10:18

jerryshao reviewed Oct 29, 2024

View reviewed changes

yuqi1129 commented Oct 29, 2024

View reviewed changes

yuqi1129 added 3 commits October 29, 2024 19:46

Optimize method toHadoopConfigMap and other fix.

b0d45f1

Add UT for FileSystemUtils.toHadoopConfigMap

11a3146

Add UT for FileSystemUtils.toHadoopConfigMap

218acae

jerryshao reviewed Oct 29, 2024

View reviewed changes

Add VisibleForTesting annotation for public fields in xxxxFileSystemP…

8ff278f

…rovider and add more comments.

yuqi1129 added 2 commits October 29, 2024 22:44

Fix java docs problem.

4f90525

Fix

bf65415

jerryshao approved these changes Oct 30, 2024

View reviewed changes

jerryshao merged commit b5a3723 into apache:main Oct 30, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5220] improvment(hadoop-catalog): Optimize the name properties keys for Hadoop catalog. #5244

[#5220] improvment(hadoop-catalog): Optimize the name properties keys for Hadoop catalog. #5244

yuqi1129 commented Oct 24, 2024

jerryshao Oct 28, 2024

yuqi1129 Oct 29, 2024 •

edited

Loading

jerryshao left a comment •

edited

Loading

jerryshao Oct 29, 2024

jerryshao Oct 29, 2024

yuqi1129 Oct 29, 2024

jerryshao Oct 29, 2024

jerryshao Oct 29, 2024

yuqi1129 Oct 29, 2024

jerryshao Oct 29, 2024

yuqi1129 Oct 29, 2024

jerryshao Oct 29, 2024

yuqi1129 Oct 29, 2024

jerryshao Oct 29, 2024

yuqi1129 commented Oct 29, 2024

jerryshao Oct 29, 2024

jerryshao Oct 29, 2024

yuqi1129 Oct 29, 2024

yuqi1129 Oct 29, 2024

yuqi1129 Oct 29, 2024

jerryshao Oct 29, 2024 •

edited

Loading

yuqi1129 Oct 29, 2024

jerryshao Oct 29, 2024

yuqi1129 Oct 29, 2024

jerryshao commented Oct 29, 2024

		@@ -95,10 +96,8 @@ protected String defaultBaseLocation() {

		protected void createCatalog() {


		private static final String OSS_FILESYSTEM_IMPL = "fs.oss.impl";

		public static final Map<String, String> GRAVITINO_KEY_TO_OSS_HADOOP_KEY =

[#5220] improvment(hadoop-catalog): Optimize the name properties keys for Hadoop catalog. #5244

[#5220] improvment(hadoop-catalog): Optimize the name properties keys for Hadoop catalog. #5244

Conversation

yuqi1129 commented Oct 24, 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

yuqi1129 Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

jerryshao left a comment • 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

yuqi1129 commented Oct 29, 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

Choose a reason for hiding this comment

jerryshao Oct 29, 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

jerryshao commented Oct 29, 2024

yuqi1129 Oct 29, 2024 •

edited

Loading

jerryshao left a comment •

edited

Loading

jerryshao Oct 29, 2024 •

edited

Loading