[#5139] feat(python-client): Support GCS fileset in the python GVFS client #5160

yuqi1129 · 2024-10-16T13:41:03Z

What changes were proposed in this pull request?

Support GCS fileset in the Python GVFS client
Add an IT about the GCS fileset.

Why are the changes needed?

It's user needs.

Fix: #5139

Does this PR introduce any user-facing change?

Modify the Python GVFS client.

How was this patch tested?

Test locally and add an IT that can't run automatically.

Modify mode as the following picture and execute ./gradlew :clients:client-python:test -PskipDockerTests=false success.

…fileset catalog

…_5019

…_5139

yuqi1129 · 2024-10-17T13:43:26Z

@jerryshao @xloya
This PR is ready for review, please help to take a look.

jerryshao · 2024-10-17T14:40:46Z

@xloya would you please help to review this code, thanks.

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

xloya · 2024-10-18T01:53:31Z

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

@@ -27,6 +27,8 @@
 from fsspec.implementations.arrow import ArrowFSWrapper
 from fsspec.utils import infer_storage_options
 from pyarrow.fs import HadoopFileSystem
+from pyarrow.fs import GcsFileSystem


In the current implementation, that all storage Python lib dependencies will be introduced into PyGVFS. Although there will be no conflicts in most cases, it may be better for users to only load the underlying storage dependencies they need. I wonder if there is a better way to introduce other FileSystem dependencies on demand, could you take a time to research this?

Okay, I will do it today.

@xloya
I have updated the code and used importlib to dynamically import file system classes according to needs.

xloya · 2024-10-21T02:15:15Z

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

+        data_dir = os.path.join(cls.gravitino_home, "data")
+        if os.path.exists(data_dir):
+            logger.info("Remove Gravitino data directory: %s", data_dir)
+            subprocess.run(["rm", "-rf", data_dir], check=False)


It's better to use shutil.rmtree to delete the dir recursively.

xloya · 2024-10-21T02:15:42Z

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

+        data_dir = os.path.join(gravitino_home, "data")
+        if os.path.exists(data_dir):
+            logger.info("Remove Gravitino data directory: %s", data_dir)
+            subprocess.run(["rm", "-rf", data_dir], check=False)


Same as above to use shutil.rmtree.

xloya · 2024-10-21T02:18:12Z

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

+
+        # Disable the following test case as it is not working for GCS
+        # >>> gcs.mkdir('example_qazwsx/catalog/schema/fileset3')
+        # >>> r = gcs.modified('example_qazwsx/catalog/schema/fileset3')


I think you should throw an exception when call modified method for fileset on gcs, rather than disable tests here.

I don't believe we need to make exceptions here as we call gcs.modified('example_qazwsx/catalog/schema/fileset3'). GCP does not support fetching the modification time of a directory, it only support getting the modification time of a object

>>> print(gcs.modified('example_qazwsx/test_gvfs_catalog7222/test_gvfs_schema/robots.txt')) 2024-10-21 07:14:27.628000+00:00 >>> print(gcs.modified('example_qazwsx/test_gvfs_catalog7222/test_gvfs_schema')) None

I will change the test code.

xloya · 2024-10-21T02:22:54Z

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

+        cls._get_gravitino_home()
+
+        cls.hadoop_conf_path = f"{cls.gravitino_home}/catalogs/hadoop/conf/hadoop.conf"
+        # append the hadoop conf to server


It seems will not append gcs conf for the server?

Yes, I will remove it.

yuqi1129 · 2024-10-21T07:41:36Z

@xloya
All comments have been resolved. Please help to take a look again.

xloya · 2024-10-21T11:09:12Z

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

+    def setUpClass(cls):
+        cls._get_gravitino_home()
+
+        cls.hadoop_conf_path = f"{cls.gravitino_home}/catalogs/hadoop/conf/hadoop.conf"


If we do not need to append some hadoop configurations in the server side, is there having any necessity to reset hadoop conf here?

If we do not need to append some hadoop configurations in the server side, is there having any necessity to reset hadoop conf here?

I'm afraid things may not be so ideal, I've encountered several cases where there are already some configuration keys in the conf file added by HDFS test cases, for example, we terminated the tests halfway and the file content can't be properly reset, so I will always rewrite it to ensure that the configuration file is clean.

xloya · 2024-10-21T11:10:03Z

Overall LGTM, but a minor issue.

…GVFS client (apache#5160) ### What changes were proposed in this pull request? - Support GCS fileset in the Python GVFS client - Add an IT about the GCS fileset. ### Why are the changes needed? It's user needs. Fix: apache#5139 ### Does this PR introduce _any_ user-facing change? Modify the Python GVFS client. ### How was this patch tested? Test locally and add an IT that can't run automatically. Modify mode as the following picture and execute `./gradlew :clients:client-python:test -PskipDockerTests=false` success. <img width="1332" alt="image" src="https://github.com/user-attachments/assets/e3b781fa-3ac1-458a-9224-d7e01107828b"> <img width="1530" alt="image" src="https://github.com/user-attachments/assets/f53c2d20-c253-4eb9-a84f-4c13569b513b">

yuqi1129 and others added 30 commits September 25, 2024 19:33

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

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

Fix python ut error again.

f4041ec

Merge branch 'issue_5019' of github.com:yuqi1129/gravitino into issue…

66247ab

…_5019

Fix test error again.

3cfb94f

yuqi1129 added 9 commits October 17, 2024 11:36

Resolve comments.

67dbc3a

Fix the java doc problem.

70a545e

Merge branch 'issue_5074' into issue_5139

4d54acf

rebase issue_5074

15bbf99

Optimize code.

cfcc544

Merge branch 'issue_5074' of github.com:yuqi1129/gravitino into issue…

acf51e1

…_5139

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

11f9992

Remove s3 related code.

4f00a2f

fix

b9ef8f0

yuqi1129 requested a review from xloya October 17, 2024 13:42

yuqi1129 self-assigned this Oct 17, 2024

xloya reviewed Oct 18, 2024

View reviewed changes

yuqi1129 added 6 commits October 18, 2024 16:49

try to import lazily.

9f65fb5

format code.

4defcc6

fix

3a907f4

fix

76912b7

fix

4478673

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

5a194df

xloya reviewed Oct 21, 2024

View reviewed changes

yuqi1129 added 2 commits October 21, 2024 10:44

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

c8b5c7c

Resolve comments

6ff9353

xloya reviewed Oct 21, 2024

View reviewed changes

jerryshao changed the title ~~[#5139] feat: (python-client): Support GCS fileset in the python GVFS client~~ [#5139] feat(python-client): Support GCS fileset in the python GVFS client Oct 21, 2024

jerryshao approved these changes Oct 21, 2024

View reviewed changes

jerryshao merged commit ef44021 into apache:main Oct 21, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5139] feat(python-client): Support GCS fileset in the python GVFS client #5160

[#5139] feat(python-client): Support GCS fileset in the python GVFS client #5160

yuqi1129 commented Oct 16, 2024 •

edited

Loading

yuqi1129 commented Oct 17, 2024

jerryshao commented Oct 17, 2024

xloya Oct 18, 2024

yuqi1129 Oct 18, 2024

yuqi1129 Oct 18, 2024

xloya Oct 21, 2024

yuqi1129 Oct 21, 2024

xloya Oct 21, 2024

yuqi1129 Oct 21, 2024

xloya Oct 21, 2024

yuqi1129 Oct 21, 2024

xloya Oct 21, 2024

yuqi1129 Oct 21, 2024

yuqi1129 Oct 21, 2024

yuqi1129 commented Oct 21, 2024

xloya Oct 21, 2024

yuqi1129 Oct 21, 2024

xloya Oct 21, 2024

xloya commented Oct 21, 2024

[#5139] feat(python-client): Support GCS fileset in the python GVFS client #5160

[#5139] feat(python-client): Support GCS fileset in the python GVFS client #5160

Conversation

yuqi1129 commented Oct 16, 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 Oct 17, 2024

jerryshao commented Oct 17, 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

yuqi1129 commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xloya commented Oct 21, 2024

yuqi1129 commented Oct 16, 2024 •

edited

Loading