[#5582] improvement(hadoop3-filesystem): Remove configuration `fs.gvfs.filesystem.providers` from GVFS client. by yuqi1129 · Pull Request #5634 · apache/gravitino

yuqi1129 · 2024-11-20T14:32:58Z

What changes were proposed in this pull request?

Configuration fs.gvfs.filesystem.providers is redundant, so we'd better remove this configuation.

Why are the changes needed?

This configuration is redundant.
Fix: #5582

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Existing tests.

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

jerryshao · 2024-12-04T08:59:42Z

I think we should not simply delete this configuration, it would be better to 1) make it deprecated; 2) keep the compatible if user set this configuration; 3) warn user that this configuration is deprecated.

yuqi1129 · 2024-12-04T09:14:00Z

I think we should not simply delete this configuration, it would be better to 1) make it deprecated; 2) keep the compatible if user set this configuration; 3) warn user that this configuration is deprecated.

I'm not 100% clear on why we only need to deprecate it. The change in PR is a compatibility modification, and the previous one was not very elegant in design.

for the user which uses fs.gvfs.filesystem.providers in 0.7.0 will definitely work in 0.8.0 after this change is merged.

jerryshao · 2024-12-04T09:15:48Z

I think we should not simply delete this configuration, it would be better to 1) make it deprecated; 2) keep the compatible if user set this configuration; 3) warn user that this configuration is deprecated.

I'm not 100% clear on why we only need to deprecate it. The change in PR is a compatibility modification, and the previous one was not very elegant in design.

for the user which uses fs.gvfs.filesystem.providers in 0.7.0 will definitely work in 0.8.0 after this change is merged.

Can you please explain more why it is compatible?

yuqi1129 · 2024-12-04T09:29:32Z

I think we should not simply delete this configuration, it would be better to 1) make it deprecated; 2) keep the compatible if user set this configuration; 3) warn user that this configuration is deprecated.

I'm not 100% clear on why we only need to deprecate it. The change in PR is a compatibility modification, and the previous one was not very elegant in design.
for the user which uses fs.gvfs.filesystem.providers in 0.7.0 will definitely work in 0.8.0 after this change is merged.

Can you please explain more why it is compatible?

In 0.7.0, If the user does not specify fs.gvfs.filesystem.providers , it won't work even though the corresponding bundle jar has been placed in the classpath.

After this PR is merged, if users:

Assign fs.gvfs.filesystem.providers a value like s3, this configuration will not take effect, as long as the AWS-bundle are in the classpath, the GVFS client can always use it.
Do not set fs.gvfs.filesystem.providers a value, the GVFS client will automatically load all file system providers in the classpath, things are also okay.

The only difference is that if users put aws-bundle and gcs-bundle in the classpath, and then they do not assign a value to fs.gvfs.filesystem.providers, Gravitino will throw exceptions in 0.7 but work well in 0.8.0. In my opinion, the behavior in 0.8.0 is more acceptable and reasonable. GVFS clients should be transparent to file system providers.

Please correct me if I'm wrong, thanks.

jerryshao · 2024-12-04T11:34:22Z

I think we should not simply delete this configuration, it would be better to 1) make it deprecated; 2) keep the compatible if user set this configuration; 3) warn user that this configuration is deprecated.

I'm not 100% clear on why we only need to deprecate it. The change in PR is a compatibility modification, and the previous one was not very elegant in design.
for the user which uses fs.gvfs.filesystem.providers in 0.7.0 will definitely work in 0.8.0 after this change is merged.

Can you please explain more why it is compatible?

In 0.7.0, If the user does not specify fs.gvfs.filesystem.providers , it won't work even though the corresponding bundle jar has been placed in the classpath.

After this PR is merged, if users:

Assign fs.gvfs.filesystem.providers a value like s3, this configuration will not take effect, as long as the AWS-bundle are in the classpath, the GVFS client can always use it.

Do not set fs.gvfs.filesystem.providers a value, the GVFS client will automatically load all file system providers in the classpath, things are also okay.

The only difference is that if users put aws-bundle and gcs-bundle in the classpath, and then they do not assign a value to fs.gvfs.filesystem.providers, Gravitino will throw exceptions in 0.7 but work well in 0.8.0. In my opinion, the behavior in 0.8.0 is more acceptable and reasonable. GVFS clients should be transparent to file system providers.

Please correct me if I'm wrong, thanks.

I see, thanks for the explanation.

jerryshao · 2024-12-04T11:38:51Z

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

    ServiceLoader<FileSystemProvider> allFileSystemProviders =
        ServiceLoader.load(FileSystemProvider.class);

+    if (null == fileSystemProviders) {


Since we already removed this fileSystemProviders configuration, why do we still need to pass in this argument?

I think we can simplify this method since we load all the fs providers.

This method will be also utilized by Gravitino server, in the server side, file system providers are required if we want to access cloud storage

I would suggest you to figure out a better way, use null value to check and differentiate different code path is not a good way.

jerryshao · 2024-12-04T11:44:49Z

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

+                }
+                resultMap.put(fileSystemProvider.scheme(), fileSystemProvider);
+              });
+      return resultMap;


Do we also need to add HDFS and local FS provider here?

ServiceLoader<FileSystemProvider> allFileSystemProviders = ServiceLoader.load(FileSystemProvider.class);

The code above will load HDFS and Local FS automatically.

yuqi1129 · 2024-12-04T14:13:25Z

FileSystemProvider is defined in moudle catalog-hadoop, which is why module filesystem-hadoop3 depends on catalog-hadoop, we can remove this dependency when we move FileSystemProvider to a common module like hadooop-common, is that okay for you?

yuqi1129 · 2024-12-04T14:13:36Z

FileSystemProvider is defined in moudle catalog-hadoop, which is why module filesystem-hadoop3 depends on catalog-hadoop, we can remove this dependency when we move FileSystemProvider to a common module like hadooop-common, is that okay for you?

@jerryshao

jerryshao · 2024-12-05T10:11:44Z

FileSystemProvider is defined in moudle catalog-hadoop, which is why module filesystem-hadoop3 depends on catalog-hadoop, we can remove this dependency when we move FileSystemProvider to a common module like hadooop-common, is that okay for you?

Yeah, this looks reasonable.

tengqm

overall lgtm.

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

jerryshao · 2024-12-13T04:11:24Z

catalogs/catalog-hadoop/build.gradle.kts

    exclude(group = "*")
  }

+  implementation(project(":catalogs:hadoop-common")) {


Make this alphabetically ordered.

jerryshao · 2024-12-13T04:11:43Z

clients/filesystem-hadoop3/build.gradle.kts

  compileOnly(libs.hadoop3.common)
-  implementation(project(":catalogs:catalog-hadoop")) {
+
+  implementation(project(":catalogs:hadoop-common")) {


Remove configuration fs.gvfs.filesystem.providers from GVFS client.

56d209a

yuqi1129 self-assigned this Nov 20, 2024

tengqm reviewed Nov 21, 2024

View reviewed changes

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

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

Polish error message in FileSystemUtils

f4a7d3f

yuqi1129 requested a review from jerryshao November 29, 2024 02:02

jerryshao reviewed Dec 4, 2024

View reviewed changes

Fix

7d18bc9

yuqi1129 added 2 commits December 4, 2024 22:26

fix test error.

6e516df

fix

2bea913

yuqi1129 added 2 commits December 9, 2024 10:45

Add a new module hadoop-common to place FileSystemProvider.java

50e652a

Rebase main and resolve conflicts.

c7e277c

tengqm reviewed Dec 9, 2024

View reviewed changes

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

Add a comma.

649bb25

yuqi1129 requested a review from jerryshao December 10, 2024 12:56

jerryshao reviewed Dec 13, 2024

View reviewed changes

order the dependencies items.

b9e03e0

jerryshao approved these changes Dec 16, 2024

View reviewed changes

jerryshao merged commit 1d92626 into apache:main Dec 16, 2024

Conversation

yuqi1129 commented Nov 20, 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?

Uh oh!

Uh oh!

Uh oh!

jerryshao commented Dec 4, 2024

Uh oh!

yuqi1129 commented Dec 4, 2024

Uh oh!

jerryshao commented Dec 4, 2024

Uh oh!

yuqi1129 commented Dec 4, 2024

Uh oh!

jerryshao commented Dec 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuqi1129 commented Dec 4, 2024

Uh oh!

yuqi1129 commented Dec 4, 2024

Uh oh!

jerryshao commented Dec 5, 2024

Uh oh!

tengqm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants