[#5996] feat(python-client): Using credentail in python GVFS client. #5997

yuqi1129 · 2024-12-26T02:08:45Z

What changes were proposed in this pull request?

Support using credentail in GVFS python client for cloud storage.

Why are the changes needed?

It's need.

Fix: #5996

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

New it and test locally

…s_support_credential

…upport_credential

yuqi1129 · 2024-12-31T14:17:20Z

@FANNG1 @jerryshao
Please help to review this PR.

clients/client-python/gravitino/api/credential/adls_token_credential.py

…upport_credential

FANNG1 · 2025-01-06T11:30:21Z

clients/client-python/tests/integration/test_gvfs_with_gcs_credential.py

+
+        cls.fs = GCSFileSystem(token=cls.key_file)
+
+    def test_mkdir(self):


why not move these tests to TestGvfsWithHDFS ?

TestGvfsWithHDFS already has the method and test_gvfs_with_gcs_credential.py just overwrite it.

Could you provide the reason?

GCS has different behaviour compared to hdfs, so we to overwrite it. OSS and S3 are similar.

could you add detailed reason in the code about why adding the test in separate file.

I can add some description about the detailed reason, not only mkdir, other operation like makedirs, ls also have similar issues.

I have added some details on why we need to overwrite it.

clients/client-python/tests/integration/test_gvfs_with_gcs_credential.py

clients/client-python/gravitino/filesystem/gvfs.py

FANNG1 · 2025-01-06T14:41:16Z

clients/client-python/gravitino/filesystem/gvfs.py

+            importlib.import_module("s3fs").S3FileSystem(
+                key=aws_access_key_id,
+                secret=aws_secret_access_key,
+                endpoint_url=aws_endpoint_url,


according to the pydoc, aws_endpoint_url seems not required?

endpoint_url : string (None) Use this endpoint_url, if specified. Needed for connecting to non-AWS S3 buckets. Takes precedence over `endpoint_url` in client_kwargs.

For Python client, the endpoint is optional, but for Java clients, this is a required value to ensure consistency.

java.lang.IllegalArgumentException: Aliyun OSS endpoint should not be null or empty. Please set proper endpoint with 'fs.oss.endpoint'. at org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystemStore.initialize(AliyunOSSFileSystemStore.java:150) at org.apache.hadoop.fs.aliyun.oss.AliyunOSSFileSystem.initialize(AliyunOSSFileSystem.java:349) at org.apache.hadoop.fs.FileSystem.createFileSystem(FileSystem.java:3469) at org.apache.hadoop.fs.FileSystem.access$300(FileSystem.java:174) at org.apache.hadoop.fs.FileSystem$Cache.getInternal(FileSystem.java:3574) at org.apache.hadoop.fs.FileSystem$Cache.get(FileSystem.java:3521) at org.apache.hadoop.fs.FileSystem.get(FileSystem.java:540) at org.apache.hadoop.fs.Path.getFileSystem(Path.java:365) at org.apache.gravitino.filesystem.hadoop.integration.test.GravitinoVirtualFileSystemIT.testDelete(GravitinoVirtualFileSystemIT.java:238)

Yes, aliyun OSS is required, but this is aws

Using S3FileSystem does not need endpoint parameters, but when applying GVFS to padas (or other third-party application), it's required, this has been tested by jerry.

Why the following code doesn't need aws_endpoint_url?

if isinstance(credential, S3SecretKeyCredential): fs = importlib.import_module("s3fs").S3FileSystem( key=credential.access_key_id(), secret=credential.secret_access_key(), ) return (expire_time, fs)

clients/client-python/gravitino/filesystem/gvfs.py

FANNG1 · 2025-01-06T14:45:46Z

clients/client-python/gravitino/filesystem/gvfs.py

+                    ),
+                )
+
+        oss_endpoint_url = self._options.get(GVFSConfig.GVFS_FILESYSTEM_OSS_ENDPOINT)


what's the difference of oss_endpoint_url and oss_endpoint?

They are the same thing, what do you mean here, oss_endpoint_url is not a good name?

If they are the same, why use two variables?

The former one is from the catalog (fetching from Gravitino server) and the other is set by the user from the client.

The action is a little wired, you only use oss endpoint only when credential is enabled in server side. seems the client side doesn't need to configuration it .

clients/client-python/gravitino/filesystem/gvfs.py

FANNG1 · 2025-01-07T07:39:34Z

clients/client-python/gravitino/filesystem/gvfs.py

+            importlib.import_module("s3fs").S3FileSystem(
+                key=aws_access_key_id,
+                secret=aws_secret_access_key,
+                endpoint_url=aws_endpoint_url,


Why the following code doesn't need aws_endpoint_url?

if isinstance(credential, S3SecretKeyCredential): fs = importlib.import_module("s3fs").S3FileSystem( key=credential.access_key_id(), secret=credential.secret_access_key(), ) return (expire_time, fs)

FANNG1 · 2025-01-07T07:46:31Z

clients/client-python/gravitino/filesystem/gvfs.py

+                    ),
+                )
+
+        oss_endpoint_url = self._options.get(GVFSConfig.GVFS_FILESYSTEM_OSS_ENDPOINT)


The action is a little wired, you only use oss endpoint only when credential is enabled in server side. seems the client side doesn't need to configuration it .

clients/client-python/gravitino/filesystem/gvfs.py

FANNG1 · 2025-01-07T07:53:32Z

clients/client-python/gravitino/filesystem/gvfs.py

+    def _get_expire_time_by_ratio(self, expire_time: int):
+        if expire_time <= 0:
+            return TIME_WITHOUT_EXPIRATION
+        return time.time() * 1000 + (expire_time - time.time() * 1000) * 0.9


0.9 seems too high, since the filesystem is used out of the control of GVFS, how about make it configurable with default 0.5?

Do you mean the credential should be refreshed within 30 minutes, even if the expiration time is an hour?

Another point, I think you have similar comments in the Java implementation, from 0.5 to 0.9. What are your considerations regarding this point?

what's the the influence if we adjust the value from 0.5 to 0.9?

If change to 0.9, Only 6 mins are left to use the filesystem.

I have added a new configuration item as suggested.

clients/client-python/tests/integration/test_gvfs_with_s3_credential.py

clients/client-python/tests/integration/test_gvfs_with_gcs_credential.py

yuqi1129 · 2025-01-07T08:41:43Z

@FANNG1

You can comment L100 in GravitinoVirtualFileSystemOSSIT and see what happens

I have said that I just want to keep the configuration for OSS in Java and Python consistent 0.7.0

yuqi1129 · 2025-01-07T08:47:32Z

For S3, I have tested again. Both Java and Python clients can omit the endpoint, I'm hesitant to change this old behaviour, maybe another PR to optimize it will be proper.

FANNG1 · 2025-01-07T13:31:53Z

I have said that I just want to keep the configuration for OSS in Java and Python consistent 0.7.0

I agree OSS endpoint is required, What I doubt is in same cases you use OSS endpoint from the server side, in another condition you use OOS endpoint from the client side.

FANNG1 · 2025-01-07T13:33:57Z

For S3, I have tested again. Both Java and Python clients can omit the endpoint, I'm hesitant to change this old behaviour, maybe another PR to optimize it will be proper.

After this PR, does client side need to configuration some catalog properties like region, endpoint?

yuqi1129 · 2025-01-08T06:12:54Z

All have been resolved. @FANNG1 please help to take a look again, thanks.

FANNG1 · 2025-01-08T07:10:06Z

clients/client-python/gravitino/filesystem/gvfs_config.py

+    # This configuration marks the expired time of the credential. For instance, if the credential
+    # fetched from Gravitino server has expired time of 3600 seconds, and the credential_expired_time_ration is 0.5
+    # then the credential will be considered as expired after 1800 seconds and will try to fetch a new credential.
+    GVFS_FILESYSTEM_CREDENTIAL_EXPIRED_TIME_RATIO = "credential_expired_time_ratio"


credential_expire_ratio?

It's also okay for me, I can make changes. Could you please help verify the accuracy of the description above?

credential will be considered as expired seems not correct, it's something like remove credential from the cache, use credential_cache_expire_ratio like sever side?

There is already a configuration item cache_expired_time to control the expiration time of the value in the cache, I'm not very in favor of credential_cache_expire_ratio.

clients/client-python/gravitino/filesystem/gvfs.py

FANNG1 · 2025-01-08T07:20:37Z

LGTM except minor comments, @jerryshao do you have time to review again?

jerryshao · 2025-01-08T07:27:22Z

You can make a final call if you think it's OK. @FANNG1

docs/how-to-use-gvfs.md

…ient. (apache#5997) ### What changes were proposed in this pull request? Support using credentail in GVFS python client for cloud storage. ### Why are the changes needed? It's need. Fix: apache#5996 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? New it and test locally

polish

9afdfa8

yuqi1129 marked this pull request as draft December 26, 2024 02:09

yuqi1129 added 2 commits December 26, 2024 19:36

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

b8d9329

…s_support_credential

polish

f17cefc

yuqi1129 marked this pull request as ready for review December 27, 2024 06:12

yuqi1129 added 4 commits December 27, 2024 14:13

fix

7a0d426

fix cyclic problem of python-client.

62170f1

fix cyclic problem of python-client.

a42677c

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

d33cb5c

…upport_credential

yuqi1129 self-assigned this Dec 27, 2024

yuqi1129 added 2 commits December 27, 2024 20:54

fix

47f267c

fix

3dbcf8f

yuqi1129 requested review from FANNG1 and jerryshao December 27, 2024 13:39

yuqi1129 added 3 commits December 30, 2024 19:40

fix

a62913c

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

dda3598

…upport_credential

fix

c8543ad

yuqi1129 mentioned this pull request Jan 2, 2025

[#6031] extend S3 credential provider to support S3 fileset operations #6033

Merged

FANNG1 reviewed Jan 3, 2025

View reviewed changes

clients/client-python/gravitino/api/credential/adls_token_credential.py Outdated Show resolved Hide resolved

yuqi1129 requested a review from FANNG1 January 3, 2025 11:43

yuqi1129 added 3 commits January 3, 2025 21:58

fix

5e69bf5

fix

7b3869a

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

bd8618e

…upport_credential

FANNG1 reviewed Jan 6, 2025

View reviewed changes

Fix

75ca62e

FANNG1 reviewed Jan 6, 2025

View reviewed changes

yuqi1129 added 3 commits January 7, 2025 09:36

fix

b007571

fix

c974e17

fix

d0b7486

FANNG1 reviewed Jan 7, 2025

View reviewed changes

clients/client-python/tests/integration/test_gvfs_with_gcs_credential.py Outdated Show resolved Hide resolved

fix

32ce066

yuqi1129 added 3 commits January 8, 2025 09:30

fix

7674bbc

Fix

dbef3d0

Fix

60e670d

yuqi1129 requested a review from FANNG1 January 8, 2025 06:13

FANNG1 reviewed Jan 8, 2025

View reviewed changes

Fix

134bf0b

FANNG1 reviewed Jan 8, 2025

View reviewed changes

docs/how-to-use-gvfs.md Outdated Show resolved Hide resolved

Fix error in default value for credential_expiration_ratio

e24fa5a

FANNG1 approved these changes Jan 8, 2025

View reviewed changes

FANNG1 merged commit 598bc05 into apache:main Jan 8, 2025
28 checks passed


		cls.fs = GCSFileSystem(token=cls.key_file)

		def test_mkdir(self):

[#5996] feat(python-client): Using credentail in python GVFS client. #5997

[#5996] feat(python-client): Using credentail in python GVFS client. #5997

Conversation

yuqi1129 commented Dec 26, 2024 • edited Loading

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 Dec 31, 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

yuqi1129 Jan 7, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi1129 commented Jan 7, 2025 • edited Loading

yuqi1129 commented Jan 7, 2025

FANNG1 commented Jan 7, 2025

FANNG1 commented Jan 7, 2025

yuqi1129 commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Jan 8, 2025

jerryshao commented Jan 8, 2025

yuqi1129 commented Dec 26, 2024 •

edited

Loading

yuqi1129 Jan 7, 2025 •

edited

Loading

yuqi1129 commented Jan 7, 2025 •

edited

Loading

FANNG1 Jan 8, 2025 •

edited

Loading