[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluginized 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.

jerryshao · 2024-09-26T01:49:37Z

build.gradle.kts

@@ -744,7 +744,8 @@ tasks {
      if (!it.name.startsWith("catalog") &&
        !it.name.startsWith("authorization") &&
        !it.name.startsWith("client") && !it.name.startsWith("filesystem") && !it.name.startsWith("spark") && !it.name.startsWith("iceberg") && it.name != "trino-connector" &&
-        it.name != "integration-test" && it.name != "hive-metastore-common" && !it.name.startsWith("flink")
+        it.name != "integration-test" && it.name != "hive-metastore-common" && !it.name.startsWith("flink") &&
+        it.name != "hadoop-gcs-bundle" && it.name != "hadoop-s3-bundle"


better renaming to "gcs-bundle" and "aws-bundle", like this, it may not only about storage, it also includes the security and other things.

xiaozcy · 2024-09-26T02:02:43Z

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

+  @Override
+  public Configuration getConfiguration(Map<String, String> conf) {
+    Configuration configuration = new Configuration();
+    conf.forEach(configuration::set);


Why do you pass all of the configurations into the hadoop conf? I think it's enough to pass in the configurations that start with gravitino.bypass in the default implementation.

It seems that the current implementation is putting all configurations including keys without the prefix gravitino.bypass

I think it's not appropriate, it might be a mistake before, can you confirm this?

Yeah, I have changed the logic and only those with prefix gravitino.bypass can pass to the Hadoop configuration, it's indeed more reasonable.

I found the line 137 of HadoopCatalogOperations still passes all the configuration into the Hadoop conf, this should be removed either.

xiaozcy · 2024-09-26T02:06:40Z

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

@@ -132,6 +132,20 @@ public void initialize(
                    Map.Entry::getValue));
    bypassConfigs.forEach(hadoopConf::set);


The initialization of Hadoop conf here seems to be useless. It's better to move these codes above into the DefaultConfigurationProvider.

Yeah, you are right, I will do it according to your suggestion.

yuqi1129 · 2024-09-30T07:42:57Z

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

+   * @return The FileSystem instance.
+   * @throws IOException If the FileSystem instance cannot be created.
+   */
+  FileSystem getFileSystem(Configuration configuration, Path path) throws IOException;


@jerryshao
Please take a look at this PR, thanks.

Why do we need to provide a Path here?

I noticed that most methods can get a FileSystem in https://github.com/apache/hadoop/blob/a9b7913d5689cae804fbd072a03cd2d2f771ab6a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java#L536-L571 do support specify the path explicitly, so I added it.

Indeed, we can also use configuration fs.defaultFs to replace it. If you feel it's an excessive value, I can remove it.

I think the current interface design is highly dependent on the Hadoop's implementation, like Configuration and Path, which is not a good design, also lose the flexibility to support some non-hadoop fs, a good interface should not rely on the third party libraries as possible as we can.

I would be inclined to only rely on FileSystem, like:

interface FileSystemProvider { FileSystem getFileSystem(Map<String, String> conf); String scheme(): } Using "conf" should be enough to create a filesystem you wanted.

jerryshao · 2024-09-30T11:52:06Z

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

+  public String getScheme() {
+    return "s3a";
+  }
+}


Can you please remove gcs, s3 related code, we can add them later on.

jerryshao · 2024-09-30T11:55:11Z

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

@@ -742,4 +778,27 @@ private boolean checkSingleFile(Fileset fileset) {
          fileset.name());
    }
  }
+
+  static FileSystem getFileSystem(Path path, Configuration conf) throws IOException {


I think we don't have use Path to create a FileSystem, Configuration itself should be enough.

yuqi1129 · 2024-09-30T12:18:42Z

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

+public class HDFSFileSystemProvider implements FileSystemProvider {
+
+  @Override
+  public FileSystem getFileSystem(Configuration configuration, Path path) throws IOException {


@jerryshao
Can you please help me verify if it's okay to initialize a HDFS file system? I can do little for it compared to the default method.

jerryshao · 2024-10-08T11:03:02Z

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

+   *
+   * @return The scheme of this FileSystem provider.
+   */
+  default String getScheme() {


It doesn't make sense to return "file" as default, as there's no default filesystem implementation.

Yeah, I removed the default implementation.

jerryshao · 2024-10-08T11:17:05Z

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

+   * @return The FileSystem instance.
+   * @throws IOException If the FileSystem instance cannot be created.
+   */
+  FileSystem getFileSystem(Configuration configuration, Path path) throws IOException;


I think the current interface design is highly dependent on the Hadoop's implementation, like Configuration and Path, which is not a good design, also lose the flexibility to support some non-hadoop fs, a good interface should not rely on the third party libraries as possible as we can.

I would be inclined to only rely on FileSystem, like:

interface FileSystemProvider { FileSystem getFileSystem(Map<String, String> conf); String scheme(): } Using "conf" should be enough to create a filesystem you wanted.

jerryshao · 2024-10-09T03:17:30Z

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

+  public FileSystem getFileSystem(Map<String, String> config) throws IOException {
+    Configuration configuration = new Configuration();
+    config.forEach(configuration::set);
+    return FileSystem.get(FileSystem.getDefaultUri(configuration), configuration);


I don't think this will get a local filesystem if the default setting is not local filesystem.

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.

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

xiaozcy reviewed Sep 26, 2024

View reviewed changes

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

jerryshao reviewed Sep 30, 2024

View reviewed changes

yuqi1129 commented Sep 30, 2024

View reviewed changes

fix

013f5cb

jerryshao reviewed Oct 8, 2024

View reviewed changes

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

Polish code.

278fcd8

yuqi1129 self-assigned this Oct 9, 2024

yuqi1129 added 2 commits October 9, 2024 18:54

fix

3fb55ad

fix

cd04666

yuqi1129 mentioned this pull request Oct 9, 2024

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

Open

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 pluginized manner for fileset catalog #5020

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

yuqi1129 commented Sep 25, 2024

yuqi1129 commented Sep 25, 2024

jerryshao Sep 26, 2024

xiaozcy Sep 26, 2024

yuqi1129 Sep 26, 2024

xiaozcy Sep 26, 2024

yuqi1129 Sep 26, 2024

xiaozcy Sep 26, 2024

xiaozcy Sep 26, 2024

yuqi1129 Sep 26, 2024

yuqi1129 Sep 30, 2024

jerryshao Sep 30, 2024

yuqi1129 Sep 30, 2024

jerryshao Oct 8, 2024

jerryshao Sep 30, 2024

yuqi1129 Sep 30, 2024

jerryshao Sep 30, 2024

yuqi1129 Sep 30, 2024

jerryshao Oct 8, 2024

yuqi1129 Oct 9, 2024

jerryshao Oct 8, 2024

jerryshao Oct 9, 2024

jerryshao commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024

		@@ -132,6 +132,20 @@ public void initialize(
		Map.Entry::getValue));
		bypassConfigs.forEach(hadoopConf::set);

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

Are you sure you want to change the base?

[#5019] feat: (hadoop-catalog): Add a framework to support multi-storage in a pluginized 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

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

Choose a reason for hiding this comment

jerryshao commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024

yuqi1129 commented Oct 9, 2024