[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluggable manner for fileset catalog #5020

yuqi1129 · 2024-09-25T11:37:34Z

What changes were proposed in this pull request?

Add a framework to support multiple storage system within Hadoop catalog

Why are the changes needed?

Some users want Gravitino to manage file system like S3 or GCS.

Fix: #5019

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

Existing test.

…fileset catalog

yuqi1129 · 2024-09-25T11:38:40Z

@xiaozcy
Please help to take a look if this PR meets your requirements, thanks.

build.gradle.kts

...g-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/DefaultConfigurationProvider.java

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

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

bundles/s3-bundle/src/main/java/org/apache/gravitino/fileset/s3/S3FileSystemProvider.java

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

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

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

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

jerryshao · 2024-10-09T03:23:01Z

I'm OK with the interface design, please polish the code to make it ready to review, also refactor the client side GravitinoVirtualFileSystem to also leverage this interface to make the fs pluggable.

yuqi1129 · 2024-10-09T04:12:42Z

refactor the client side GravitinoVirtualFileSystem to also leverage this interface to make the fs pluggable

I will use another PR to refactor the Java client for the fileset.

yuqi1129 · 2024-10-09T10:59:24Z

I'm OK with the interface design, please polish the code to make it ready to review, also refactor the client side GravitinoVirtualFileSystem to also leverage this interface to make the fs pluggable.

@jerryshao
I have updated the PR and please help to review it again, thanks.

yuqi1129 · 2024-10-15T07:08:56Z

@jerryshao
The code has been updated.

jerryshao

Can you please carefully review your code for several times to avoid any typos, polish your code structure and simplify the logic as possible as you can.

jerryshao · 2024-10-15T08:05:08Z

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

+        (String)
+            propertiesMetadata
+                .catalogPropertiesMetadata()
+                .getOrDefault(config, HadoopCatalogPropertiesMetadata.DEFAULT_FS_PROVIDER);


What is the default value for this property, should if be "file"?

jerryshao · 2024-10-15T08:06:37Z

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

+      }
+
+      LOG.warn(
+          "Can't get schema from path: {} and default filesystem provider is null, using"


jerryshao · 2024-10-15T08:09:40Z

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

+      return getFileSystemByScheme(LOCAL_FILE_SCHEME, config, path);
+    }
+
+    return getFileSystemByScheme(path.toUri().getScheme(), config, path);


if (scheme == null && defaultFSProvider == null) { LOG.warn(xxx) } newScheme = scheme == null ? defaultFSProvider.getScheme() : scheme; getFileSystemByScheme(newScheme);

jerryshao · 2024-10-15T08:11:08Z

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

@@ -90,6 +90,10 @@ public class HadoopCatalogOperations implements CatalogOperations, SupportsSchem

  private CatalogInfo catalogInfo;

+  private final Map<String, FileSystemProvider> fileSystemProvidersMap = Maps.newHashMap();
+
+  private String defaultFilesystemProvider;


You can maintain a default FileSystemProvider, not just a string. Besides, it is FileSystem, not FileSystem.

jerryshao · 2024-10-15T08:12:23Z

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

+      throws IOException {
+    FileSystemProvider provider = fileSystemProvidersMap.get(scheme);
+    if (provider == null) {
+      throw new IllegalArgumentException("Unsupported scheme: " + scheme);


You should clearly tell user why the exception is happened, the code here is not easy for user to know the actual reason.

jerryshao · 2024-10-15T08:12:50Z

...adoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogPropertiesMetadata.java

+              PropertyEntry.stringOptionalPropertyEntry(
+                  DEFAULT_FS_PROVIDER,
+                  "Default file system provider, used to create the default file system "
+                      + "candidate value is 'local', 'hdfs' or others specified in the "


file, not local...

jerqi · 2024-10-15T09:50:41Z

pluginized -> plugable.

jerryshao · 2024-10-15T12:52:20Z

clients/filesystem-hadoop3/build.gradle.kts

@@ -71,6 +75,11 @@ tasks.build {
  dependsOn("javadoc")
 }

+tasks.compileJava {


Why do we need this?

This module depends on catalog-hadoop, however, due to the fact that the output directory of task :catalogs:catalog-hadoop:jar and :catalogs:catalog-hadoop:runtimeJars are the same (lib/build/), so we need to add these two dependency or gradle compile will fail, the same goes for others module

tasks.test { val skipITs = project.hasProperty("skipITs") if (skipITs) { exclude("**/integration/test/**") } else { dependsOn(":catalogs:catalog-hadoop:jar", ":catalogs:catalog-hadoop:runtimeJars") dependsOn(":catalogs:catalog-hive:jar", ":catalogs:catalog-hive:runtimeJars") dependsOn(":catalogs:catalog-kafka:jar", ":catalogs:catalog-kafka:runtimeJars") } }

...adoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogPropertiesMetadata.java

jerryshao · 2024-10-15T13:02:00Z

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

+    this.defaultFileSystemProviderScheme =
+        StringUtils.isNotBlank(defaultFileSystemProviderClassName)
+            ? FileSystemUtils.getSchemeByFileSystemProvider(
+                defaultFileSystemProviderClassName, fileSystemProvidersMap)
+            : LOCAL_FILE_SCHEME;


You can create the default fs provider and get scheme from default fs provider, no need to use this logic.

If we use the service load mechanism to load file system providers, I can't get why we need to provide the configuration filesystem-providers, please see below

// fileSystemProviders is useless if we use ServiceLoader public static Map<String, FileSystemProvider> getFileSystemProviders(String fileSystemProviders) { Map<String, FileSystemProvider> resultMap = Maps.newHashMap(); ServiceLoader<FileSystemProvider> fileSystemProvidersLoader = ServiceLoader.load(FileSystemProvider.class); fileSystemProvidersLoader.forEach( fileSystemProvider -> resultMap.put(fileSystemProvider.name(), fileSystemProvider)); return resultMap; }

then the only configuration we need to provide is default-filesystem-provider, @jerryshao what do you think about it, is it okay for you?

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

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

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

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

...adoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogPropertiesMetadata.java

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

jerryshao · 2024-10-16T05:44:19Z

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

@@ -125,6 +132,10 @@ public void initialize(URI name, Configuration configuration) throws IOException

    initializeClient(configuration);

+    // Register the default local and HDFS FileSystemProvider
+    String fileSystemProviders = configuration.get(FS_FILESYSTEM_PROVIDERS);
+    fileSystemProvidersMap.putAll(FileSystemUtils.getFileSystemProviders(fileSystemProviders));


gvfs will depend on the hadoop-catalog, can you please check the dependencies of gvfs to make sure the shading jar is correct?

Some existing IT has checked the current code, moreover, I have verified this logic when developing python GVFS client.

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

jerryshao · 2024-10-16T05:58:46Z

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

@@ -125,6 +132,10 @@ public void initialize(URI name, Configuration configuration) throws IOException

    initializeClient(configuration);

+    // Register the default local and HDFS FileSystemProvider
+    String fileSystemProviders = configuration.get(FS_FILESYSTEM_PROVIDERS);


Do you also need a default fileSystemProvider for gvfs?

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

docs/hadoop-catalog.md

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

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

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

d2447a2

…fileset catalog

yuqi1129 added 4 commits September 25, 2024 19:51

Fix compile distribution error.

7e5a8b5

fix

f53c5ef

fix

36fedcd

fix

e93fba5

jerryshao reviewed Sep 26, 2024

View reviewed changes

build.gradle.kts Outdated Show resolved Hide resolved

xiaozcy reviewed Sep 26, 2024

View reviewed changes

...g-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/DefaultConfigurationProvider.java Outdated Show resolved Hide resolved

...atalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java Outdated Show resolved Hide resolved

yuqi1129 added 3 commits September 26, 2024 12:10

fix

b1e04b6

Changed according to comments.

db00e65

fix

c793582

yuqi1129 commented Sep 30, 2024

View reviewed changes

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/FileSystemProvider.java Outdated Show resolved Hide resolved

jerryshao reviewed Sep 30, 2024

View reviewed changes

bundles/s3-bundle/src/main/java/org/apache/gravitino/fileset/s3/S3FileSystemProvider.java Outdated Show resolved Hide resolved

jerryshao reviewed Sep 30, 2024

View reviewed changes

...atalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java Outdated Show resolved Hide resolved

yuqi1129 commented Sep 30, 2024

View reviewed changes

...alog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/HDFSFileSystemProvider.java Outdated Show resolved Hide resolved

fix

013f5cb

jerryshao reviewed Oct 8, 2024

View reviewed changes

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/FileSystemProvider.java Outdated Show resolved Hide resolved

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/FileSystemProvider.java Outdated Show resolved Hide resolved

yuqi1129 added 2 commits October 8, 2024 21:40

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

dba5753

resolve comments.

16dfc73

jerryshao reviewed Oct 9, 2024

View reviewed changes

...log-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/LocalFileSystemProvider.java Outdated Show resolved Hide resolved

Polish code.

278fcd8

yuqi1129 self-assigned this Oct 9, 2024

yuqi1129 added 2 commits October 9, 2024 18:54

fix

3fb55ad

fix

cd04666

This was referenced Oct 9, 2024

[#5074] feat(hadoop-catalog): Support GCS fileset. #5079

Merged

[Improvement] Use the plugin storage framework for GVFS client #5097

Closed

Change gvfs accordingly.

ffaa064

jerryshao reviewed Oct 15, 2024

View reviewed changes

yuqi1129 added 2 commits October 15, 2024 17:15

fix

5dbca5f

Update code.

f27520a

jerryshao changed the title ~~[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluginized manner for fileset catalog~~ [#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluggable manner for fileset catalog Oct 15, 2024

jerryshao reviewed Oct 15, 2024

View reviewed changes

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

jerryshao reviewed Oct 15, 2024

View reviewed changes

yuqi1129 added 2 commits October 16, 2024 10:49

Fix

27bc2ab

Fix

9dc0f5a

yuqi1129 requested a review from jerryshao October 16, 2024 03:57

jerryshao reviewed Oct 16, 2024

View reviewed changes

Fix

2ee1709

jerryshao reviewed Oct 16, 2024

View reviewed changes

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

jerryshao reviewed Oct 16, 2024

View reviewed changes

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/FileSystemUtils.java Outdated Show resolved Hide resolved

Fix

05e5d20

jerryshao reviewed Oct 16, 2024

View reviewed changes

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/FileSystemUtils.java Outdated Show resolved Hide resolved

jerryshao reviewed Oct 16, 2024

View reviewed changes

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/FileSystemUtils.java Outdated Show resolved Hide resolved

jerryshao reviewed Oct 16, 2024

View reviewed changes

...ain/java/org/apache/gravitino/filesystem/hadoop/GravitinoVirtualFileSystemConfiguration.java Outdated Show resolved Hide resolved

jerryshao reviewed Oct 16, 2024

View reviewed changes

docs/hadoop-catalog.md Outdated Show resolved Hide resolved

yuqi1129 added 2 commits October 16, 2024 14:24

Fix

8f28211

Fix

35cba1e

jerryshao reviewed Oct 16, 2024

View reviewed changes

...atalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java Outdated Show resolved Hide resolved

jerryshao reviewed Oct 16, 2024

View reviewed changes

...ogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/fs/FileSystemUtils.java Outdated Show resolved Hide resolved

Fix

bcf2f12

jerryshao approved these changes Oct 16, 2024

View reviewed changes

jerryshao merged commit b06b511 into apache:main Oct 16, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluggable manner for fileset catalog #5020

[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluggable manner for fileset catalog #5020

yuqi1129 commented Sep 25, 2024

yuqi1129 commented Sep 25, 2024

jerryshao commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024

yuqi1129 commented Oct 15, 2024

jerryshao left a comment

jerryshao Oct 15, 2024

jerryshao Oct 15, 2024

jerryshao Oct 15, 2024

jerryshao Oct 15, 2024

jerryshao Oct 15, 2024

jerryshao Oct 15, 2024

jerqi commented Oct 15, 2024

jerryshao Oct 15, 2024

yuqi1129 Oct 15, 2024

jerryshao Oct 15, 2024

yuqi1129 Oct 15, 2024

jerryshao Oct 16, 2024

yuqi1129 Oct 16, 2024

jerryshao Oct 16, 2024

[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluggable manner for fileset catalog #5020

[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluggable manner for fileset catalog #5020

Conversation

yuqi1129 commented Sep 25, 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?

yuqi1129 commented Sep 25, 2024

jerryshao commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024

yuqi1129 commented Oct 15, 2024

jerryshao 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

jerqi commented Oct 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment